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

Handle routes that may have trailing slash #249

Closed
wants to merge 5 commits into from

Conversation

fuller
Copy link

@fuller fuller commented Nov 29, 2019

Currently we are seeing a situation where hitting a route directly like example.com/about will work as expected, but example.com/about/ is hitting the _error.js file since the paths do not match.

This changes strips the trailing slash before doing path comparisons so that page will be found, then using canonical urls we can get search engines to understand as well.

I'm sure there are better solutions to this problem as well, but this has worked for us in the meantime.

@fuller fuller changed the title Handle routes that may have extraenous trailing slash Handle routes that may have trailing slash Nov 29, 2019
@danielcondemarin
Copy link
Contributor

@fuller Thank you for the PR!

I'd like to avoid regexes whenever possible in the Lambda Handlers, they could lead to DoS attacks, perf issues etc.

Would something like this work instead:

if (path[path.length - 1] === '/') { 
  // strip last / char
  path = path.slice(0, -1)
}

Also a simple test case would be great!

@danielcondemarin
Copy link
Contributor

I will hold this off until vercel/next.js#6421 (comment) is resolved! At the moment the component behaves in the same way that the next server does so will keep it like that for now.

@danielcondemarin
Copy link
Contributor

danielcondemarin commented Feb 16, 2020

Closing as there is no activity in vercel/next.js#6421 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants