-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Bare paths redirect to paths with a trailing slash in production #8593
fix: Bare paths redirect to paths with a trailing slash in production #8593
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a test please? Can you restore the issue template please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have to add a couple of tests. For example, you added a 400
code and a 301
code, and we should create two test cases that hit those cases.
They were already there :) but happy to add tests. |
Aiming to get this done by tomorrow. |
switch (trailingSlash) { | ||
case 'never': { | ||
// Redirect to a url with no trailingSlash if the incoming url had a trailingSlash | ||
if (pathname.endsWith('/')) { | ||
res.statusCode = 301; | ||
location.pathname = pathnameWithoutSlash; | ||
res.setHeader('Location', location.toString()); | ||
res.end(location); | ||
} | ||
break; | ||
} | ||
case 'always': { | ||
// Redirect to a url with trailingSlash if the incoming url did not have a trailingSlash | ||
if (!pathname.endsWith('/')) { | ||
res.statusCode = 301; | ||
location.pathname = pathnameWithSlash; | ||
res.setHeader('Location', location.toString()); | ||
res.end(location); | ||
} | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised that TS doesn't throw an error, but trailingSlash
has three values: always, never and ignore. We need to handle ignore
.
Also, I believe we should return 404, because that's current behaviour of the dev server.
#9080 was aiming to fix the same bug but since it was made later than this PR, we would have taken yours to fix the bug. However, it turns out your fix won't actually work so we asked PR 9080 author to make changes in core instead. |
That's great! I was blocked with other tasks, appreciate your patience! |
fix #7808
Changes
Fix bare paths redirect to paths with a trailing slash in production
Testing
Docs