Skip to content
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

Closed
wants to merge 14 commits into from
Closed

fix: Bare paths redirect to paths with a trailing slash in production #8593

wants to merge 14 commits into from

Conversation

rishi-raj-jain
Copy link
Contributor

@rishi-raj-jain rishi-raj-jain commented Sep 18, 2023

fix #7808

Changes

Fix bare paths redirect to paths with a trailing slash in production

Testing

Docs

@changeset-bot
Copy link

changeset-bot bot commented Sep 18, 2023

⚠️ No Changeset found

Latest commit: c494915

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@rishi-raj-jain rishi-raj-jain changed the title drill trailingSlash fix: Bare paths redirect to paths with a trailing slash in production Sep 18, 2023
@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Sep 18, 2023
Copy link
Member

@ematipico ematipico left a 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?

Copy link
Member

@ematipico ematipico left a 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.

packages/integrations/node/src/http-server.ts Outdated Show resolved Hide resolved
packages/integrations/node/src/http-server.ts Outdated Show resolved Hide resolved
packages/integrations/node/src/http-server.ts Outdated Show resolved Hide resolved
@rishi-raj-jain
Copy link
Contributor Author

you added a 400 code and a 301 code,

They were already there :) but happy to add tests.

@rishi-raj-jain
Copy link
Contributor Author

Aiming to get this done by tomorrow.

Comment on lines +72 to +92
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;
}
Copy link
Member

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.

@florian-lefebvre
Copy link
Member

#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.

@rishi-raj-jain rishi-raj-jain deleted the bare-standalone-paths branch December 15, 2023 14:29
@rishi-raj-jain
Copy link
Contributor Author

#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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bare paths redirect to paths with a trailing slash in production
3 participants