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: spa option, preview and dev for MPA and SSR apps #8217

Merged
merged 21 commits into from
May 23, 2022

Conversation

brillout
Copy link
Contributor

@brillout brillout commented May 18, 2022

Description

This fixes $ vite preview and $ vite dev for SSR and MPA apps.

This PR enables users and frameworks to set config.isSPA = false to disable SPA catch-all routing.

The docs added by this PR include a description of what "SPA catch-all routing" means.

It supersedes these previous PRs:

And fixes these issues:

Additional context

I decided against config.appType: spa | ssr | mpa because many frameworks blur the line between ssr and mpa. For example, vite-plugin-ssr 0.4 allows the user to implement SSR as well as MPA apps. It also allows the user to have one SSR page while the rest is MPA; it's then not clear whether the app type is ssr or mpa. Many other frameworks, such as Nuxt, also blur the line between SSR and MPA.

Thus I believe having only two modes "SPA" or "not SPA" is the way to go.

I'm fairly confident this PR is in a good state and can be merged as-is on a high-level (edit). (But happy to be shown wrong.)

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 patak-dev added the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label May 18, 2022
@patak-dev patak-dev added this to the 3.0 milestone May 18, 2022
docs/config/index.md Outdated Show resolved Hide resolved
brillout and others added 4 commits May 18, 2022 19:12
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@antfu
Copy link
Member

antfu commented May 20, 2022

A bit nitpicking here, I personally don't feel the isSPA name convention matches with the rest of the configs. Maybe simply spa: false?

@patak-dev
Copy link
Member

We talked about the PR in today's team meeting. We're worried about making Vite preview ever more complex to accommodate future requirements from users like vite-plugin-ssr. We still think that a spa option (I agree with Anthony's rename request) is a necessity given current hardcoded assumptions in Vite (independent of the preview changes).
So let's move forward with the PR in v3, but frameworks using vite preview instead of a wrapper needs to understand that future features need to be done in middleware. It is out of the scope of Vite to keep augmenting vite preview to support other requirements.

@brillout
Copy link
Contributor Author

I've updated the PR and I believe it resolves all discussions.

I've refactored how we document this: the documentation now lives under Guide - SSR which I believe to be a more natural fit.

We talked in today's meeting about transformIndexHtml. We should document that this hook isn't called during SSR.

This is somewhat documented at https://vitejs.dev/guide/ssr.html#setting-up-the-dev-server and since the docs now live on that same page, I believe it's enough.

We're worried about making Vite preview ever more complex to accommodate future requirements from users [...] but frameworks using vite preview instead of a wrapper needs to understand that future features need to be done in middleware

I agree and this is actually what this PR is about: getting Vite's internal SPA assumptions out of the way in order to uncouple Vite from user middlewares.

Telefunc now also adds its RPC middleware to the development and preview servers and it works. I believe that, after this PR, things will be flexible enough to accommodate for all kinds of needs. Some minor tweaks may be needed, but nothing fundamental AFAICT. And, yes, for unreasonable requirements we can answer: write your own CLI.

docs/config/index.md Outdated Show resolved Hide resolved
brillout and others added 2 commits May 22, 2022 11:24
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
benmccann
benmccann previously approved these changes May 22, 2022
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
benmccann
benmccann previously approved these changes May 22, 2022
Copy link
Collaborator

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for driving this and seeing it through. I'm really excited about it!

@brillout
Copy link
Contributor Author

Alright, I've applied all suggestions. Let me know if I missed something. Also feel free to directly push to the PR.

Awesome! Thanks for driving this and seeing it through. I'm really excited about it!

I'm glad you're excited about this. Thank you both for your helpful reviews.

patak-dev
patak-dev previously approved these changes May 22, 2022
docs/guide/ssr.md Outdated Show resolved Hide resolved
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@patak-dev patak-dev changed the title fix: $ vite preview and $ vite dev for MPA and SSR apps feat: spa option, preview and dev for MPA and SSR apps May 23, 2022
@patak-dev patak-dev merged commit d7cba46 into vitejs:main May 23, 2022
@brillout
Copy link
Contributor Author

brillout commented Jun 2, 2022

This will likely by superseded by #8438.

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

Successfully merging this pull request may close these issues.

5 participants