-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
fix(server): handle appType mpa html fallback #10336
Conversation
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.
Thanks!
We should also attribute brillout before merging:
Co-authored-by: brillout <github@brillout.com>
(I think i got the email right)
Co-authored By: brillout <github@brillout.com>
7c5b5b4
to
a43b021
Compare
@bluwy added the attribution on the commit 👍 |
Ah looks like it should be |
@bluwy no need to be sorry, I did not check as well 👀 . Okay then lets just add it on the squash commit message 👍 |
@bluwy @patak-dev now that all issues in milestone 3.2 is done, can we expect a 3.2 release soon ? |
@jiby-aurum we are going to release 3.2-beta.0 today, if things look good, ask for wider testing on beta.1 and release the stable version after ViteConf (prob ~12days from now). But you could use the beta today. |
@patak-dev that's perfect, thanks ! |
if (spaFallback) { | ||
return `/index.html` | ||
} |
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'm getting errors when setting appType: 'mpa'
and requesting a URI from the dev server that doesn't exist (and would fall back to index.html
in the SPA case). Instead of returning a simple 404, as expected, it's a hmr overlay that says:
Cannot read properties of undefined (reading 'charAt')
at file:///usr/src/client/node_modules/vite/dist/node/chunks/dep-51c4f80a.js:59865:27
at viteHtmlFallbackMiddleware (file:///usr/src/client/node_modules/vite/dist/node/chunks/dep-51c4f80a.j
I couldn't track back the code in that chunk to it's source (not sure which package this is from?), but it does this:
if(rewriteTarget.charAt(0) !== '/') {
logger(
'We recommend using an absolute path for the rewrite target.',
'Received a non-absolute rewrite target',
rewriteTarget,
'for URL',
req.url
);
}
Clearly, it expects the rewrite to return a string in any case. But the lines I commented on, will return undefined
for mpa
.
I can fix this by just returning an empty string in the mpa case. I'm not sure whether that's the right thing to do, though?
Commenting here, because this change introduced the bug.
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.
Would you create a minimal reproduction and an issue in the repo so we don't lose track of this issue? You can link to this PR and comment, that is helpful to know already the connection.
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.
Done in #10966.
Description
Fix
appType: 'mpa'
which currently doesn't work:indexHtmlMiddleware()
doesn't work withoutspaFallbackMiddleware()
. In other words MPAs also needspaFallbackMiddleware()
.This seems to have been a glitch we missed at #8452.
close #9865
Additional context
This is a blocker for Telefunc.
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).