-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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
Improve rewrites documentation #34725
Conversation
destination: 'https://example.com/blog/', | ||
}, | ||
{ | ||
source: '/blog/:path(.+)', |
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.
This seems like a bug, wdyt @ijjk 🤔
source: '/blog/:path(.+)', | |
source: '/blog/:path(.+)', |
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.
For context, I discovered that :path*
doesn't catch trailing slashes so Davis came up with a wildcard expression that does.
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.
We should be handling the trailing slash here
next.js/packages/next/lib/load-custom-routes.ts
Lines 72 to 73 in 1a0d149
// we add an optional trailing slash at the end for easier | |
// configuring between trailingSlash: true/false |
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.
@ijjk yep! Here's a reproduction. Click on the "not working" demo link below, navigate to a specific blog post, then reload the page. It will redirect infinitely.
If you want to take a look at the code, here are the full repos. They both have trailingSlash: true
:
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.
Oh it looks like the destination
for this rewrite needs to end with a trailingSlash to match the other deployments expectation e.g. does changing it to the below resolve this?
destination: `${BLOG_URL}/blog/:path*/`
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 documenting that the destination
for a rewrite needs to honor the external destination's trailingSlash config makes sense
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.
Good catch, it works when you include a trailing slash for both the source and destination like this.
{
source: "/blog/:path*/",
destination: `${BLOG_URL}/blog/:path*/`,
},
I'm assuming this is better than the :path(.+)
wildcard so I just updated the PR to reflect that.
Co-authored-by: JJ Kasper <jj@jjsweb.site>
This is a PR to update the rewrites documentation. This is after struggling to get rewrites to work with
trailingSlash
for a customer as this wasn't documented. The main culprit was the:path*
wildcard not catching trailing slashes. The changes made to for this commit were:trailingSlash
Context
For a reproduction of the
:path*
wildcard not catching trailing slashes as expected, see below. Click on the "not working" demo link below, navigate to a specific blog post, then reload the page. It will redirect infinitely.:path*
:path(.+)
If you want to take a look at the code, here are the full repos. They both have
trailingSlash: true
: