Repository navigation
fix(node): strip trailing slashes when resolving view names - #18372
kamilmysliwiec merged 3 commits into
Conversation
kamilmysliwiec
left a comment
kamilmysliwiec
left a comment
There was a problem hiding this comment.
Thanks! The bug is real (render('users/') fails on master, while Express renders users/index.ejs), but stripping the slash resolves to a different file than Express does when both exist.
With views/a.ejs and views/a/index.ejs, Express 5 renders a/index.ejs for render('a/'); with this change it renders a.ejs. A trailing slash names the directory, so it should only look up <view>/index.<ext> (that is, skip the a.ejs candidate rather than fall back to it). It would also be good to:
- cover that case in the spec (a file and a directory with the same name);
- keep
/alone from turning into.ejsat the views root.
|
Thanks for the clear feedback and great catch, @kamilmysliwiec! I've updated the implementation in commit \e802febab:
|
| private async resolveFile(view: string, extension: string): Promise<string> { | ||
| const hasTrailingSlash = /[/\\]$/.test(view); | ||
| const cleanView = view.replace(/[/\\]+$/, ''); | ||
| const ext = extname(cleanView) || `.${extension}`; | ||
|
|
||
| let candidates: string[] = []; | ||
| if (cleanView) { | ||
| if (hasTrailingSlash) { | ||
| // A trailing slash names the directory, so only look up <view>/index.<ext> | ||
| candidates = [join(cleanView, `index${ext}`)]; | ||
| } else { | ||
| // "users" is "users.ext", then "users/index.ext", as with Express. Unlike | ||
| // Express, which tries both in each directory in turn, a file in any | ||
| // directory wins over an index | ||
| const fileCandidate = extname(cleanView) | ||
| ? cleanView | ||
| : `${cleanView}${ext}`; | ||
| const { dir, name, ext: candidateExt } = parse(fileCandidate); | ||
| candidates = [fileCandidate, join(dir, name, `index${candidateExt}`)]; | ||
| } | ||
| } | ||
|
|
||
| for (const candidate of candidates) { |
There was a problem hiding this comment.
render('a.ejs/') worked on master and works on Express (gives a.ejs, or users/index.ejs for users.ejs/), here it throws Failed to lookup view, because slash is cut before extension is checked
Express gets all these cases from one thing: extname('a/') is empty so name becomes a/.ejs, and basename('.ejs', '.ejs') is empty so index is a/index.ejs. Same here is shorter and needs no seperate branch for slash, it also needs basename in import from path:
| private async resolveFile(view: string, extension: string): Promise<string> { | |
| const hasTrailingSlash = /[/\\]$/.test(view); | |
| const cleanView = view.replace(/[/\\]+$/, ''); | |
| const ext = extname(cleanView) || `.${extension}`; | |
| let candidates: string[] = []; | |
| if (cleanView) { | |
| if (hasTrailingSlash) { | |
| // A trailing slash names the directory, so only look up <view>/index.<ext> | |
| candidates = [join(cleanView, `index${ext}`)]; | |
| } else { | |
| // "users" is "users.ext", then "users/index.ext", as with Express. Unlike | |
| // Express, which tries both in each directory in turn, a file in any | |
| // directory wins over an index | |
| const fileCandidate = extname(cleanView) | |
| ? cleanView | |
| : `${cleanView}${ext}`; | |
| const { dir, name, ext: candidateExt } = parse(fileCandidate); | |
| candidates = [fileCandidate, join(dir, name, `index${candidateExt}`)]; | |
| } | |
| } | |
| for (const candidate of candidates) { | |
| private async resolveFile(view: string, extension: string): Promise<string> { | |
| const ext = extname(view) || `.${extension}`; | |
| const fileName = extname(view) ? view : `${view}${ext}`; | |
| const { dir, base } = parse(fileName); | |
| // "users" is "users.ext", then "users/index.ext", as with Express. Unlike | |
| // Express, which tries both in each directory in turn, a file in any | |
| // directory wins over an index. For "users/", base is only the extension, | |
| // so the index is "users/index.ext" | |
| for (const candidate of [ | |
| fileName, | |
| join(dir, basename(base, ext), `index${ext}`), | |
| ]) { |
Compared it with Express 5 on 13 view names and 6 layouts, only difference is v1.2/ where Express throws about missing engine and here it is lookup error. a/ next to a.ejs still dont fall back to a.ejs, dotfile a/.ejs is tried before index as Express does, / and \ still dont resolve. Same cases as in your spec pass with it
Test for it:
it('should keep the file for a trailing slash after an extension, as with Express', async () => {
write('views/a.ejs', 'a file');
write('views/users/index.ejs', 'users index');
renderer.setEngine({ engine: { ejs }, root: views });
await expect(renderer.render('a.ejs/')).resolves.toBe('a file');
await expect(renderer.render('users.ejs/')).resolves.toBe('users index');
});
Stripping the trailing slash before reading the extension made
render('a.ejs/') look for a.ejs/index.ejs and fail, although master and
Express render a.ejs. Build the file name the way Express does instead:
"a/" becomes "a/.ejs", whose index is "a/index.ejs", so a directory
still wins over a same-named file and "/" stays outside the views root.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
When rendering a view name with a trailing slash (e.g. @Render('users/') or
enderer.render('users/')), createRenderFn appends the file extension directly, creating a path like 'users/.ejs'. In Node, path.parse('users/.ejs') treats .ejs as the basename (
ame: '.ejs', ext: ''), which causes
esolveFile to look for 'users/.ejs' and 'users/.ejs/index' rather than 'users/index.ejs', throwing an error.
What is the new behavior?
Trailing slashes are stripped before constructing the candidate filename, allowing lookups like 'users/' to correctly resolve to 'users/index.ejs' and match Express behavior.
Does this PR introduce a breaking change?