Skip to content

fix(server): handle appType mpa html fallback#10336

Merged
patak-cat merged 2 commits intovitejs:mainfrom
gb-jos:fix/appType-map
Oct 5, 2022
Merged

fix(server): handle appType mpa html fallback#10336
patak-cat merged 2 commits intovitejs:mainfrom
gb-jos:fix/appType-map

Conversation

@gb-jos
Copy link
Contributor

@gb-jos gb-jos commented Oct 3, 2022

Description

Fix appType: 'mpa' which currently doesn't work: indexHtmlMiddleware() doesn't work without spaFallbackMiddleware(). In other words MPAs also need spaFallbackMiddleware().

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?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@gb-jos gb-jos mentioned this pull request Oct 3, 2022
9 tasks
@gb-jos
Copy link
Contributor Author

gb-jos commented Oct 3, 2022

@bluwy this is basically #9865 with the review updates made

bluwy
bluwy previously approved these changes Oct 4, 2022
Copy link
Member

@bluwy bluwy left a 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)

@bluwy bluwy changed the title fix: appType mpa fix(server): handle appType mpa html fallback Oct 4, 2022
@bluwy bluwy added feat: html p3-minor-bug An edge case that only affects very specific usage (priority) labels Oct 4, 2022
gb-jos and others added 2 commits October 4, 2022 14:43
Co-authored By: brillout <github@brillout.com>
@gb-jos
Copy link
Contributor Author

gb-jos commented Oct 4, 2022

@bluwy added the attribution on the commit 👍

@bluwy
Copy link
Member

bluwy commented Oct 4, 2022

Ah looks like it should be Romuald Brillout <git@brillout.com> instead. Sorry! (should've googled) We can also add the message before squash merging too, but thanks for adding it in your commit.

@gb-jos
Copy link
Contributor Author

gb-jos commented Oct 4, 2022

@bluwy no need to be sorry, I did not check as well 👀 . Okay then lets just add it on the squash commit message 👍

@bluwy bluwy added this to the 3.2 milestone Oct 5, 2022
@patak-cat patak-cat merged commit 65dd88b into vitejs:main Oct 5, 2022
@gb-jos
Copy link
Contributor Author

gb-jos commented Oct 5, 2022

@bluwy @patak-dev now that all issues in milestone 3.2 is done, can we expect a 3.2 release soon ?

@patak-cat
Copy link
Member

@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.

@gb-jos
Copy link
Contributor Author

gb-jos commented Oct 5, 2022

@patak-dev that's perfect, thanks !

Comment on lines +24 to +26
if (spaFallback) {
return `/index.html`
}

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.

Copy link
Member

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #10966.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat: html p3-minor-bug An edge case that only affects very specific usage (priority)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants