-
-
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
feat: redirect trailing slashes on on-demand rendered pages #12994
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 0fdb109 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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 PR is blocked because it contains a minor
changeset. A reviewer will merge this at the next release if approved.
CodSpeed Performance ReportMerging #12994 will not alter performanceComparing Summary
|
I wonder if this is a significant enough change to be behind an experimental flag. |
This features seems to be mostly a change for adapters. In this case, you might want to add a new Astro feature: astro/packages/astro/src/types/public/integrations.ts Lines 100 to 130 in df90e6d
Like this, adapters can opt-in and "break" things as much as they want, using their own versioning. What do you think? |
I don't think so. This will work without any change on the adapter's part. |
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 makes sense to me 👍 I don't think this could break things so probably don't need an experimental option
That I know. I was suggesting an experimental path without necessarily having one in core, since this is a change that affects only on-demand pages with an adapter, and I would prefer this way because we don't know if users have already fixed this on their end. |
@ematipico why do you think this is breaking? |
!preview trailing-slash-redirect |
Snapshots have been released for the following packages:
Publish Log
Build Log
|
For docs, would just suggest taking a quick peek to make sure nothing here affects what we say in these places: |
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.
Just noting that when I look at this entry as a whole, I think we might be at the point where a restructuring is needed:
- the first line mentions that this specifically is to configure how the dev server works, but there is also talk about "in production" below that's not particularly separated out/scannable for someone to find their use case
- we clearly define the 3 options, but then in normal paragraph text give more specifics about some of the options. This might mean that we need to separate each of the 3 options and give them all their own content
- there is a difference in how prerendered vs on demand works that's not separated out/scannable for someone to find their use case
Instead of adding on more notes/caveats, can we think of a better way to organize all this so people will be better easier to find the information they need quickly? Any thoughts on this?
@sarah11918 I totally missed that this said it was just about dev server. That's already incorrect: it's used in production too. |
5902224
to
476a95d
Compare
@sarah11918 how about now |
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
!preview trailing-slash-redirect |
Snapshots have been released for the following packages:
Publish Log
Build Log
|
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.
Approving for docs!
Changes
When the
trailingSlash
option is set toalways
ornever
, on-demand rendered pages will now redirect to the correct URL when the trailing slash is incorrect. This was previously the case for static pages, but now works for on-demand pages as well. There are some exceptions:trailingSlash
isignore
then it is not redirected/favicon.ico
will not redirect to/favicon.ico/
/_
with not redirect, because these are usually special internal pathsIn dev we don't redirect so it's more obvious that a link is incorrect. Instead I have updated the 404 page to give a bit more information if there's a trailing slash mismatch, and suggest goign to the version with the correct slash:
Fixes #12532
Fixes #12833
Fixes #11575
Testing
Added a big test suite. Refer to it for more examples.
Docs
This will need docs