Skip to content

fix: move spa fallback after postHooks#4670

Closed
benmccann wants to merge 1 commit intovitejs:mainfrom
benmccann:spa-fallback
Closed

fix: move spa fallback after postHooks#4670
benmccann wants to merge 1 commit intovitejs:mainfrom
benmccann:spa-fallback

Conversation

@benmccann
Copy link
Collaborator

@benmccann benmccann commented Aug 20, 2021

Description

Move SPA fallback after postHooks

Right now SvelteKit can't inject a middleware to handle requests because this SPA fallback intercepts them all. This would fix that. I agree with @axe-me who suggested the current behavior doesn't make a lot of sense (#4640 (comment)), but perhaps there's a use case we're not aware of?

The comment for postHooks says:

  // This is applied before the html middleware so that user middleware can
  // serve custom content instead of index.html.

The SPA fallback would need to be before postHooks for the same reason

Additional context

SvelteKit right now runs in middleware mode and has to provide its own set of server options. As a result, people get confused between SvelteKit's server options and Vite's server options. It also means a lot of duplicate functionality like https certificate handling. This change would allow SvelteKit to run a Vite server from the SvelteKit CLI. SvelteKit's middleware is then injected using a plugin with the configureServer hook.

Proof-of-concept PR is pending on the SvelteKit side that could be cleaned up and checked in after this PR is released: sveltejs/kit#2232.

This idea was originally suggested by @axe-me: #4230 (comment)

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.

@benmccann benmccann mentioned this pull request Aug 20, 2021
Closed
9 tasks
@patak-cat
Copy link
Member

patak-cat commented Aug 20, 2021 •
edited
Loading

This looks fine to me and all tests are passing. Looks like the history middleware for spa fallback was added in this PR and the comment was already there so Evan may have needed to place it before for some reason. @antfu do you see a problem with moving the spa-fallback after user middleware?

Edit: it wasn't exactly the same comment, the more explicit message was added later but the user middlewares were called directly next to the history middleware anyways.

Shinigami92
Shinigami92 reviewed Aug 20, 2021
View reviewed changes
@benmccann benmccann mentioned this pull request Aug 23, 2021
Closed
@patak-cat patak-cat added the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label Aug 25, 2021
@benmccann benmccann mentioned this pull request Aug 26, 2021
Closed
brillout
brillout approved these changes Aug 28, 2021
View reviewed changes
Copy link
Contributor

@brillout brillout left a comment

Choose a reason for hiding this comment

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

I'm not all to familiar with that part of Vite, but LGTM.

@patak-cat
Copy link
Member

patak-cat commented Aug 29, 2021

@benmccann we discussed this PR with the team and we are good to merge it if Vitepress works after this change.

But I just tried to build Vitepress using it and there is a regression. After this change you can't longer access the docs using http://localhost:3000/ and you are forced to use http://localhost:3000/index.html. To test you need to yarn link vite to vitepress and

yarn build
yarn docs-dev

So it looks like the current order is needed, we could still change it in a minor if this problem is because of a problem in the way Vitepress is handling the spa fallback and there is a fix inline. Maybe SvelteKit needs another workaround, I hope that the solution is not adding a new option here (although an option to disable the SPA fallback may be needed if everything fails?).

@benmccann
Copy link
Collaborator Author

benmccann commented Aug 29, 2021

hmm. I don't know exactly how I'd do this without a new option. open to ideas. but I think for now I will close this PR and reopen the original one that added a new option instead because I'm afraid I don't have any other ideas. please take a look at #4640

@benmccann benmccann closed this Aug 29, 2021
@benmccann benmccann mentioned this pull request May 18, 2022
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@brillout brillout brillout approved these changes

@Shinigami92 Shinigami92 Shinigami92 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

on hold p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@benmccann @patak-cat @brillout @Shinigami92