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: dont remove indexHtmlMiddleware when spa false #8438

Closed
wants to merge 1 commit into from

Conversation

patak-dev
Copy link
Member

Description

Reverts the removal of indexHtmlMiddleware when spa: false. This middleware is needed for regular MPAs in Vite. Ping @brillout @benmccann


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@brillout
Copy link
Contributor

brillout commented Jun 2, 2022

This is ok for vite-plugin-ssr but I can see it to be problematic for other SSR frameworks that use .html files.

Seems like we do need config.appType: 'spa' | 'mpa' | 'ssr' after all. SSR frameworks that blur the line between SPA/MPA/SSR will choose ssr because they'll have their own HTML rendering middleware. This is confusing: for example, there are vite-plugin-ssr users that implement MPAs and for these the value of config.appType would still be ssr even though they don't even have a server and the type of their app is clearly MPA.

Instead of config.appType, how about config.server.mode: 'spa' | 'mpa' | 'ssr' instead?

If we don't foresee another use case for config.appType then I believe we should focus only on config.server instead.

@patak-dev
Copy link
Member Author

We decided to go with a global config option because it doesn't affect only the server, it also has implications for preview (maybe later for build optimizations?). So I think that config.server.mode isn't a good fit here.

I think we should go back to appType: 'spa' | 'mpa' | 'ssr' then. This would make the current server.middlewareMode string form obsolete. That should probably be back to being a string, and then you can control the inclusion of the middlewares using the app type:

Current Proposed
server mode appType: 'spa' | 'mpa' | 'ssr'
server.middlewareMode: 'ssr' appType: 'ssr' + server.middlewareMode: true
server.middlewareMode: 'html' appType: 'spa' | 'mpa' + server.middlewareMode: true

@brillout
Copy link
Contributor

brillout commented Jun 2, 2022

I like making middlewareMode a boolean. Neat.

With config.server.mode I also mean config.preview.mode. Just like we do for other config.server/config.preview configs. (E.g. setting config.server.host also sets config.preview.host.)

The middlewareMode is about the dev server. So I still think the option should not be global and should live at config.server(/config.preview).

@patak-dev
Copy link
Member Author

I like making middlewareMode a boolean. Neat.

For reference, middlewareMode was originally a boolean and we transformed it to a string once people also wanted the HTML middlewares when not using the server.

With config.server.mode I also mean config.preview.mode. Just like we do for other config.server/config.preview configs. (E.g. setting config.server.host also sets config.preview.host.)

host is different because you may want different values for the dev server and preview. But with appType, you always want the same value. I don't think that server.mode and preview.mode should ever be different. We have other global options like base that are global too and affect both. And the appType may affect build at one point, and then you will need to specify build.mode, server.mode, preview.mode. I think a global appType is the way to go here.

The middlewareMode is about the dev server. So I still think the option should not be global and should live at config.server(/config.preview).

Yes, middlewareMode is a server only option and that makes sense. The preview always starts and listen a server now, we don't have a preview.middlewareMode option yet but it could be possible. Although I think we'll never go there.

@brillout
Copy link
Contributor

brillout commented Jun 2, 2022

Makes sense on a high-level.

I'm just a bit worried on a lower level.

the appType may affect build at one point

Let's imagine we change Vite to make $ vite build build both the client and server code.

A vite-plugin-ssr MPA user will want to have config.appType = 'MPA' to only have a client-side build. But this will be the wrong value for the dev/preview server.

Anyways, I guess it's fine to go for config.appType for now. We cannot foresee everything and I'm all for shipping & breaking fast.

For additional context: it's tempting to go for a high-level MPA/SSR/SPA setting because that's what the discourse talked about the last couple of years, but technically this may be a dead-end. Maybe I'm wrong though, we'll see. Long story short: I'm 👍 for going with config.appType.

@benmccann
Copy link
Collaborator

I'm unclear what we're talking about when we say MPA and why it needs a new option. I know that it stands for multi-page app and I guess it's not doing SSR, but why does it affect anything on the server? Shouldn't it just affect whether navigation on the client is done via pushState (SPA) or clicking a link and letting the browser handle it (MPA)? That seems like a purely client-side concern, but maybe I'm missing something?

I was assuming SPA rendered the initial request at the requested URL and then handled each subsequent URL on the client via pushState, but maybe SPA is synonymous with hash routing here and all server requests are redirected to a single URL?

@brillout
Copy link
Contributor

brillout commented Jun 2, 2022

@benmccann I think it's about this: https://vitejs.dev/guide/build.html#multi-page-app.

So:

Several .html files => MPA.

A single index.html at the root of the app => SPA.

Just like a vanilla Vite SPA, a vanilla Vite MPA also needs indexHtmlMiddleware().

@patak-dev
Copy link
Member Author

Yes, and the difference between SPA and MPA is that with MPA, sirv should not have single: true and the spaFallbackMiddleware shouldn't be included

@benmccann
Copy link
Collaborator

Thanks for the clarification!

@patak-dev's proposal from above (#8438 (comment)) sounds fine to me

@aleclarson
Copy link
Member

Should middlewareMode: true be the default when appType: 'ssr' is used?

@patak-dev
Copy link
Member Author

Should middlewareMode: true be the default when appType: 'ssr' is used?

We can't because SvelteKit is going to use appType: 'ssr' without middleware mode, as they are using the Vite server as is IIUC. Is that right @benmccann?

Interestingly, VitePress has a config option to turn it into an MPA: https://github.com/vuejs/vitepress/blob/0927a7393b3b484d49a2d06d5b41073b40cec244/src/node/config.ts#L66

@benmccann
Copy link
Collaborator

Yes, that's correct. We do SSR and use the Vite server as is

@patak-dev
Copy link
Member Author

patak-dev commented Jun 2, 2022

Something that I don't like about 'mpa' | 'spa' | 'ssr' is that you could do SSR+MPA, or SSR+SPA, and it could confuse users. 'ssr' here means: "remove all assumptions (like sirv single: true and the HTML related middlewares), I'm going to do that". Maybe 'mpa' | 'spa' | 'custom' is better?

@benmccann
Copy link
Collaborator

benmccann commented Jun 2, 2022

I'm not too bothered by 'ssr', but 'custom' is fine with me as well. I'm happy with either. I suppose I probably would lean towards 'custom' actually

@patak-dev
Copy link
Member Author

Implemented with appType: 'spa' | 'mpa' | 'custom' in:

@patak-dev
Copy link
Member Author

Closing in favor of #8452

@patak-dev patak-dev closed this Jun 17, 2022
@sapphi-red sapphi-red deleted the fix/spa-config-indexHtmlMiddleware branch October 10, 2022 06:11
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

4 participants