feat: redirect trailing slashes on on-demand rendered pages #12994
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 57feb05 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 |
github-actions
bot
added
pkg: astro
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
CodSpeed Performance ReportMerging #12994 will not alter performanceComparing Summary
|
ematipico
reviewed
Jan 16, 2025
|
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. |
bluwy
approved these changes
Jan 16, 2025
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 |
Closed
1 task
|
Snapshots have been released for the following packages:
Publish LogBuild Log |
|
For docs, would just suggest taking a quick peek to make sure nothing here affects what we say in these places: |
sarah11918
reviewed
Jan 27, 2025
There was a problem hiding this comment.
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?
packages/astro/src/template/4xx.ts
Outdated
| title: 'Not found', | ||
| tabTitle: '404: Not Found', | ||
| body: `<p>Your site is configured with <code>trailingSlash</code> set to <code>${trailingSlash}</code>. Do you want to go to <a href="${corrected}">${corrected}</a> instead?</p> | ||
| <p>Come to our <a href="https://astro.build/chat">Discord</a> if you need help.</p>`, |
There was a problem hiding this comment.
Not opposed to also a link to Discord for help, but perhaps a docs link to config reference for trailingSlash is more directly helpful?
There was a problem hiding this comment.
Updated. The message for base (which is what I copied this from) should probably also be updated, but that can go ina different PR
|
@sarah11918 I totally missed that this said it was just about dev server. That's already incorrect: it's used in production too. |
ascorbic
force-pushed
the
trailing-magic
branch
from
5902224 to
476a95d
Compare
|
@sarah11918 how about now |
sarah11918
reviewed
Jan 27, 2025
Comment on lines
+240
to
+242
| * - `'ignore'` - Match URLs regardless of whether a trailing "/" exists. Requests for "/foo" and "/foo/" will both match the same route. | ||
| * - `'always'` - Only match URLs that **include** a trailing slash (e.g: "/foo/"). In production, requests for URLs *without* a trailing slash will be redirected to the correct URL. In development, requests for URLs *without* a trailing slash will display a warning page. | ||
| * - `'never'` - Only match URLs that **do not include** a trailing slash (e.g: "/foo"). In production, requests for URLs *with* a trailing slash will be redirected to the correct URL. In development, requests for URLs *with* a trailing slash will display a warning page. |
There was a problem hiding this comment.
Suggested change
| * - `'ignore'` - Match URLs regardless of whether a trailing "/" exists. Requests for "/foo" and "/foo/" will both match the same route. | |
| * - `'always'` - Only match URLs that **include** a trailing slash (e.g: "/foo/"). In production, requests for URLs *without* a trailing slash will be redirected to the correct URL. In development, requests for URLs *without* a trailing slash will display a warning page. | |
| * - `'never'` - Only match URLs that **do not include** a trailing slash (e.g: "/foo"). In production, requests for URLs *with* a trailing slash will be redirected to the correct URL. In development, requests for URLs *with* a trailing slash will display a warning page. | |
| * - `'ignore'` - Match URLs regardless of whether a trailing "/" exists. Requests for "/about" and "/about/" will both match the same route. | |
| * - `'always'` - Only match URLs that include a trailing slash (e.g: "/about/"). In production, requests for on-demand rendered URLs without a trailing slash will be redirected to the correct URL for your convenience. However, in development, they will display a warning page reminding you that you have `always` configured. | |
| * - `'never'` - Only match URLs that do not include a trailing slash (e.g: "/about"). In production, requests for on-demand rendered URLs with a trailing slash will be redirected to the correct URL for your convenience. However, in development, they will display a warning page reminding you that you have `never` configured. |
I think much better! (And, now correct? 😅 )
Just tried something above that I'm not totally sold on, but I think provides a helpful nuance. This is my thinking, and maybe some different text gets at this better:
- taking this opportunity to exorcise yet another "foo" from our examples!
- re-iterating the on-demandness of production (lest someone not read below)
- Since we're talking about the same URLs in both sentences, probably an easier read to "However, they..." without making them read the mouthful again
- Bold/italics are generally not used for emphasis. Bold is OK when absolutely necessary and someone's eye needs to be drawn somewhere, but as a rule, italics isn't great for accessibility in docs and shouldn't really be used. (In this case, I also removed bold because it's not e.g. we want someone scanning the page to hit "with" -- that in and of itself isn't a key or meaningful idea.)
- I thought it was helpful to say why they'd get a warning page; what the purpose is. This is admittedly making things longer, though. I would ideally want to say e.g., "so that you will notice when internal links contradict your configuration" but wondered whether we could get away with something like above
sarah11918
reviewed
Jan 27, 2025
| * Use this configuration option if your production host has strict handling of how trailing slashes work or do not work. | ||
| * | ||
| * You can also set this if you prefer to be more strict yourself, so that URLs with or without trailing slashes won't work during development. | ||
| * Note that for prerendered pages, trailing slashes are handled by the hosting platform, so may not behave in the same way. |
There was a problem hiding this comment.
Suggested change
| * Note that for prerendered pages, trailing slashes are handled by the hosting platform, so may not behave in the same way. | |
| * Trailing slashes on prerendered pages are handled by the hosting platform, and may not respect your chosen configuration. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
When the
trailingSlashoption is set toalwaysornever, 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:trailingSlashisignorethen it is not redirected/favicon.icowill 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