Skip to content
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: move spa fallback after postHooks #4670

Closed
wants to merge 1 commit into from

Conversation

benmccann
Copy link
Collaborator

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.

@patak-dev
Copy link
Member

patak-dev commented Aug 20, 2021

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.

@patak-dev patak-dev added the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label Aug 25, 2021
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-dev
Copy link
Member

@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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants