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

feat: option to disable SPA catch-all #8061

Closed
wants to merge 1 commit into from

Conversation

brillout
Copy link
Contributor

@brillout brillout commented May 7, 2022

Description

Resurrection of #7665: it was merged but later reverted because SPAs should be able to do client-side routing and catch-all routing is needed for that.

Such catch-all routing doesn't make sense for MPAs or SSR apps. Hence this new option to disable it.

@benmccann This removes the SPA fallback middleware, so this may be a solution for your problem.

The name disableSPACatchAll is fairly long for Vite's standards. But I think it's worth it. I think it's pretty clear and considerably increases the probability of understanding what disableSPACatchAll is about without having to look up documentation.

Additional context

This will be used by Vike Frameworks (frameworks based on Vike tools such as vite-plugin-ssr 0.4).

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.

I considered adding a test, but I realized that it would require a whole new playground setup. I'm thinking that adding a playground just for this would add too much bloat to be worth it. Note that vite-plugin-ssr will have tests for this, so we'll catch any problems at latest when running vite-plugin-ssr's test suite.

@benmccann
Copy link
Collaborator

I would like to see something like this as well. I had filed an issue about this in #7631 hoping we could fix it in 3.0. I tried it awhile back in #4640 as well, but it didn't get much momentum so we're just removing the middlewares instead, which is more brittle.

I think viteIndexHtmlMiddleware and vite404Middleware need to be disabled for SSR as well

@brillout
Copy link
Contributor Author

brillout commented May 7, 2022

I think viteIndexHtmlMiddleware and vite404Middleware need to be disabled for SSR as well

The post hooks, and therefore the SSR middleware, are installed before these two middlewares, so I don't see how these two would conflict with the SSR middleware:

  • If the SSR middleware handles the 404, then vite404Middleware() is not called. If it doesn't then vite404Middleware() is called but that's a good thing. (Vite-plugin-ssr doesn't handle 404s if the user didn't define _error.page.js.)
  • The URL root / is handled by the SSR middleware, therefore indexHtmlMiddleware() won't be called.

I lean towards keeping them since they are needed for MPAs.

The pro about disableSPACatchAll is that it works for both MPAs and SSR. The con is that it's a brittle config if we later decide to change things (leading to a breaking change).

I'm thinking a more robust design would be a more high-level config: serveMode: 'spa' | 'mpa' | 'ssr'. This is future-proof.

We can start with 'spa' | 'ssr' and later add 'mpa' if someone wants it. (I'd be up to implement mpa but I'm not familiar with Vite's MPA ecosystem, so it's probably best if we focus on spa and ssr first. Maybe someone can ping MPA folks to engage in a larger discussion.)

Thoughts?

Also, one thing I forgot to mention: I think we shouldn't add these options to the Vite CLI. Because it should be the SSR plugin/framework that sets the config, not the user. Saving us from creating a new Vite CLI option.

@brillout
Copy link
Contributor Author

brillout commented May 7, 2022

(The failing tests are flaky tests. Things are green on my fork.)

@brillout
Copy link
Contributor Author

brillout commented May 8, 2022

I'm thinking a more robust design would be a more high-level config: serveMode: 'spa' | 'mpa' | 'ssr'.

I'm just remembering patak's idea of a global appType: 'spa' | 'mpa' | 'ssr'. I like this even more. It's an easy, clear, and future-proof contract between the user/framework and Vite.

@patak-dev
Copy link
Member

Related PR: #5757, this is also something that @Artur- wanted for Vaadin.

@brillout brillout mentioned this pull request May 9, 2022
4 tasks
@benmccann
Copy link
Collaborator

I don't see how these two would conflict with the SSR middleware
I lean towards keeping them since they are needed for MPAs.
I'm thinking a more robust design would be a more high-level config: serveMode: 'spa' | 'mpa' | 'ssr'. This is future-proof.

I just tested and SvelteKit does work if we leave these two middlewares in the general case. However, I do wonder if there is still some case where you'd want it disabled. If you add the user's application as a middleware to an Express server, they may want to add middleware handlers for other URLs and 404 and may want it disabled in this case. So it seems safer to me to disable it for the SSR case. If we go with the 'spa' | 'mpa' | 'ssr' design then maybe it makes sense to disable for SSR, but leave for MPA?

@brillout
Copy link
Contributor Author

@benmccann Makes sense 👍.

@brillout
Copy link
Contributor Author

Closing in favor of #8217.

@benmccann I've left the 404 middleware when config.isSPA === false because the post hooks run before the 404 middleware. This means SvelteKit can still render the 404 page. (FYI vite-plugin-ssr users may not define an _error.page.js which means that vite-plugin-ssr cannot render a 404 and in that case the 404 middleware is actually useful.)

@brillout brillout closed this May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants