Show injected custom 404 route in dev #6940
Merged
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: 80ca780 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 |
delucis
changed the title
github-actions
bot
added
the
pkg: astro
Dynamic catch-all routes can match against `/404/` but will then error because they’re not actually designed to handle a param of `'404'`. Testing `route` instead excludes dynamic routes (because they’ll contain dynamic segments inside square brackets). Not sure if someone might _want_ to render the 404 via a dynamic route, but we already don’t support that, so this doesn’t change anything.
delucis
commented
Apr 29, 2023
| return manifest.routes.find((r) => r.component.match(pattern)); | ||
| function getCustom404Route(manifest: ManifestData): RouteData | undefined { | ||
| const route404 = /^\/404\/?$/; | ||
| return manifest.routes.find((r) => route404.test(r.route)); |
There was a problem hiding this comment.
I think this is safe? Previously we only supported 404.astro, 404.md, 404.mdx, and 404.mdoc as custom 404 routes. Those would all have a route of /404 or /404/, so are still supported. This change adds custom 404 support for src/pages/404.html or for injectRoute({ pattern: '404', entrypoint: '...' }) in dev.
| await r.done; | ||
| const doc = await r.text(); | ||
| expect(doc).to.match(/<h1>Custom 404<\/h1>/); | ||
| expect(r.res.statusCode).to.equal(200); |
There was a problem hiding this comment.
Not quite sure whether we should expect a 404 status here or whether this request/response mock always returns 200?
This comment was marked as outdated.
This comment was marked as outdated.
matthewp
approved these changes
May 1, 2023
Merged
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
injectRoutenot used #6937getCustom404Routelogic to find 404s added viainjectRoutesrc/pages/404.html, which we don’t have currently.Testing
Added a fixture + test like we have for the current custom 404 strategies and also a dev server unit test.
I also applied the same changes in a patch inside a monorepo project to test the logic when the injected route comes from a separate package.
Docs
Bug fix, no docs needed.