Skip to content

feat: spa option, preview and dev for MPA and SSR apps#8217

Merged
patak-cat merged 21 commits intovitejs:mainfrom
brillout:fix/vite-preview
May 23, 2022
Merged

feat: spa option, preview and dev for MPA and SSR apps#8217
patak-cat merged 21 commits intovitejs:mainfrom
brillout:fix/vite-preview

Conversation

@brillout
Copy link
Contributor

@brillout brillout commented May 18, 2022 •
edited
Loading

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.

@brillout brillout mentioned this pull request May 18, 2022
Closed
9 tasks
@patak-cat patak-cat added the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label May 18, 2022
@patak-cat patak-cat added this to the 3.0 milestone May 18, 2022
benmccann
benmccann reviewed May 18, 2022
View reviewed changes
benmccann
benmccann reviewed May 18, 2022
View reviewed changes
brillout and others added 4 commits May 18, 2022 19:12
@brillout
fix: $ vite preview and $ vite dev for MPA and SSR apps
76c95db
@brillout
chore: minor code readability improvement
2f1aed3
@brillout @benmccann
chore: update docs/config/index.md
8794130 
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@brillout @benmccann
chore: update packages/vite/src/node/config.ts
e7fc218 
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
benmccann
benmccann reviewed May 18, 2022
View reviewed changes
@brillout brillout force-pushed the fix/vite-preview branch from 93a18ad to e7fc218 Compare May 18, 2022 17:15
benmccann
benmccann reviewed May 18, 2022
View reviewed changes
@brillout brillout mentioned this pull request May 18, 2022
Closed
benmccann
benmccann reviewed May 19, 2022
View reviewed changes
benmccann
benmccann reviewed May 19, 2022
View reviewed changes
benmccann
benmccann reviewed May 19, 2022
View reviewed changes
@antfu
Copy link
Member

antfu commented May 20, 2022 •
edited
Loading

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-cat
Copy link
Member

patak-cat commented May 20, 2022

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.

patak-cat
patak-cat reviewed May 20, 2022
View reviewed changes
@brillout
Copy link
Contributor Author

brillout commented May 21, 2022

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.

benmccann
benmccann reviewed May 22, 2022
View reviewed changes
brillout and others added 2 commits May 22, 2022 11:24
@brillout @benmccann
Update docs/config/index.md
70cfa5e 
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@brillout
fix: udpate JSDoc
bcfda16
@brillout brillout force-pushed the fix/vite-preview branch from 40d275e to a4c2c16 Compare May 22, 2022 12:29
@brillout brillout force-pushed the fix/vite-preview branch from a4c2c16 to 70b9e41 Compare May 22, 2022 12:32
benmccann
benmccann previously approved these changes May 22, 2022
View reviewed changes
benmccann
benmccann reviewed May 22, 2022
View reviewed changes
benmccann
benmccann reviewed May 22, 2022
View reviewed changes
@brillout @benmccann
Update packages/vite/src/node/preview.ts
65b28e7 
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
benmccann
benmccann previously approved these changes May 22, 2022
View reviewed changes
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

brillout commented May 22, 2022

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-cat
patak-cat previously approved these changes May 22, 2022
View reviewed changes
benmccann
benmccann reviewed May 22, 2022
View reviewed changes
benmccann
benmccann reviewed May 23, 2022
View reviewed changes
@patak-cat @benmccann
chore: update
bd91916 
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@patak-cat patak-cat 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-cat patak-cat merged commit d7cba46 into vitejs:main May 23, 2022
This was referenced May 23, 2022
Closed
Closed
@brillout
Copy link
Contributor Author

brillout commented Jun 2, 2022

This will likely by superseded by #8438.

@patak-cat patak-cat mentioned this pull request Jun 2, 2022
Merged
4 tasks
@brillout brillout deleted the fix/vite-preview branch September 18, 2024 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benmccann benmccann benmccann left review comments

@patak-cat patak-cat patak-cat left review comments

@bluwy bluwy bluwy approved these changes

Assignees

No one assigned

Labels

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

Projects

Team Board
Archived in project

Milestone

3.0

Development

Successfully merging this pull request may close these issues.

5 participants

@brillout @antfu @patak-cat @benmccann @bluwy