Fix: 404.html load correctly on preview #9907
Conversation
🦋 Changeset detectedLatest commit: 940f2c6 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
the
pkg: astro
ktym4a
changed the title
|
I believe this file is a good place to start for testing: https://github.com/withastro/astro/blob/main/packages/astro/test/preview-routing.test.js You might want to create a use case very similar to what was filed in the issue, and make sure it works |
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Thank you, @ktym4a; the change looks good to me! There is one thing missing: a changeset, then I think we can merge your fix :)
|
Thanks for your kind support! |
|
@lilnasy I would love your review here |
| for (const middleware of server.middlewares.stack) { | ||
| // This hardcoded name will not break between Vite versions | ||
| if ((middleware.handle as Connect.HandleFunction).name === 'vite404Middleware') { | ||
| middleware.handle = handle404; | ||
| } | ||
| } | ||
|
|
||
| next(); |
There was a problem hiding this comment.
Was it necessary to overwrite vite's middleware during the execution of our other middleware?
There was a problem hiding this comment.
@lilnasy
Thanks, I'll do a little research to see if I can move somewhere properly handle vite404Middleware.
There was a problem hiding this comment.
It's fine if you don't find it or if it takes too long. This is already quite an improvement!
There was a problem hiding this comment.
We could add a "// TODO: look into why the replacement needs to happen here" in the meanwhile.
There was a problem hiding this comment.
Added TODO comment.
I will research and send another PR if I think it can be improved.
Thank you very much.
There was a problem hiding this comment.
If we do, we may need to set appType to 'custom' and add HTML middlewares, I think.
References:
https://github.com/vitejs/vite/blob/775bb5026ee1d7e15b75c8829e7f528c1b26c493/packages/vite/src/node/preview.ts#L213-L219
https://github.com/vitejs/vite/blob/main/packages/vite/src/node/server/middlewares/notFound.ts#L3
https://vitejs.dev/config/shared-options.html#apptype
Changes
Fix: #9854
https://github.com/ktym4a/astro/blob/520be8b113bbb3189033e2952b28a6cda2a676a1/packages/astro/src/core/preview/vite-plugin-astro-preview.ts#L81-L98
I think previous
vite404Middlewarechecks were never happen becausenext();is called before that.so, Moved to be called.
Testing
If this is good, I will add test cases.
but I just need to know where to add test cases.
Docs
don't need to add.