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!: appType (spa, mpa, custom), boolean middlewareMode #8452

Merged
merged 25 commits into from Jun 18, 2022
Merged

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Jun 2, 2022

Description

Close #8438

Context:

This PR implements:

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

server.middlewareMode was originally a boolean and we extended it to be 'ssr' | 'html' once people wanted control over the included middlewares.

#8217 introduced a spa: boolean option, but we found out while discussing in #8438 that it isn't enough and that there is overlap between this new option and the extensions previously added to middlewareMode

This PR reverts middlewareMode to a boolean, that now only controls if there is a server managed by Vite and has no effect on what middlewares are included. A new appType option allows us to define at a high level the different sets of middlewares (and options like sirv single: true in preview).

I think that 'custom' is less confusing than 'ssr' for the option that removes all the HTML middlewares and leaves the HTML handling to the framework.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@patak-dev patak-dev added the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label Jun 2, 2022
docs/config/server-options.md Outdated Show resolved Hide resolved
docs/config/server-options.md Outdated Show resolved Hide resolved
docs/config/server-options.md Outdated Show resolved Hide resolved
docs/config/shared-options.md Outdated Show resolved Hide resolved
docs/config/shared-options.md Outdated Show resolved Hide resolved
docs/config/shared-options.md Outdated Show resolved Hide resolved
docs/guide/ssr.md Show resolved Hide resolved
docs/guide/ssr.md Show resolved Hide resolved
packages/vite/src/node/config.ts Outdated Show resolved Hide resolved
@@ -536,7 +544,10 @@ export async function resolveConfig(
}
},
worker: resolvedWorkerOptions,
spa: config.spa ?? true
appType:
config.appType ?? config?.server?.middlewareMode === 'ssr'
Copy link
Collaborator

Choose a reason for hiding this comment

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

middlewareMode is a boolean now

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for backward compat. We can break it directly for v3 thought

Copy link
Member

Choose a reason for hiding this comment

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

I think they are only a handful of frameworks out there using middlewareMode: 'ssr' now that it should be safe to break. Otherwise another option is to log a warning, but framework authors would likely fix it right away so that end-users don't see it.

patak-dev and others added 9 commits June 3, 2022 07:15
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@brillout
Copy link
Contributor

brillout commented Jun 3, 2022

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.

How about we make only vite.createServer() to default to middlewareMode: true when appType: 'ssr'?

While we keep middlewareMode: false for $ vite dev when appType: 'ssr'.

So that SvelteKit (and others) can still use $ vite dev instead of vite.createServer().

(Btw. I really like replacing ssr with custom. It addresses my concerns.)

CC @aleclarson.

@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.

How about we make only vite.createServer() to default to middlewareMode: true when appType: 'ssr'?

I actually like that middlewareMode only defines if the server is controlled by Vite or not, and that appType controls the type of app but has nothing to do with who manages the server. Given that this is an API that only advanced users will need I think it is better to keep middlewareMode and appType orthogonal.

@brillout
Copy link
Contributor

brillout commented Jun 3, 2022

Actually, how about a vite.createDevMiddleware()?

Before:

if (process.env.NODE_ENV === 'production') {
  app.use(sirv(`./dist/client/`))
} else {
  const vite = await import('vite')
  viteDevServer = await vite.createServer({
    server: { middlewareMode: true },
  })
  app.use(viteDevServer.middlewares)
}

After:

if (process.env.NODE_ENV === 'production') {
  app.use(sirv(`./dist/client/`))
} else {
  app.use((await import('vite')).createDevMiddleware())
}

It's low priority and I'm personally fine with the current API. But I can see many to welcome such aesthetic improvement and Vite 3 is a nice opportunity for that.

docs/config/shared-options.md Outdated Show resolved Hide resolved
docs/config/shared-options.md Outdated Show resolved Hide resolved
packages/vite/src/node/config.ts Outdated Show resolved Hide resolved
patak-dev and others added 3 commits June 3, 2022 16:23
Co-authored-by: Shinigami <chrissi92@hotmail.de>
Co-authored-by: Shinigami <chrissi92@hotmail.de>
Co-authored-by: Shinigami <chrissi92@hotmail.de>
@benmccann
Copy link
Collaborator

How about we make only vite.createServer() to default to middlewareMode: true when appType: 'ssr'?
While we keep middlewareMode: false for $ vite dev when appType: 'ssr'.
So that SvelteKit (and others) can still use $ vite dev instead of vite.createServer().

That sounds confusing to me. I think it's better if it just always have the same value. I don't mind overriding the default value if needed. The question to me though is what is the better default and I think that generally false would be preferable

Actually, how about a vite.createDevMiddleware()?

It's not a compelling need, but it might be slightly nicer. I wouldn't be opposed to it as a follow-up

benmccann
benmccann previously approved these changes Jun 3, 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.

lgtm. thank you so much for this!

brillout
brillout previously approved these changes Jun 4, 2022
@brillout
Copy link
Contributor

brillout commented Jun 4, 2022

better to keep middlewareMode and appType orthogonal.

That sounds confusing to me

👍 Ok that makes sense. You're right, it's clearer to separate both. Sorry for the noise.

As for createDevMiddleware(), I still think that it looks nicer than (await createServer({ server: { middlewareMode: true } })).middlewares. It's only aesthetics so I'm fine if we skip that part, although I do think that such aesthetics do contribute to Vite's zero-config feeling in a non-negligible way. Folks (rightfully) dread verbose configurations a la webpack.

Anyways, LGTM; green-check-marking this.

@patak-dev
Copy link
Member Author

I agree with you @brillout, it feels strange to use a createServer function that doesnt creates a server. Maybe createServerMiddlewares to keep them aligned. We can do another PR to discuss with others

@brillout
Copy link
Contributor

brillout commented Jun 4, 2022

Maybe createServerMiddlewares to keep them aligned.

👍

Although I have a slight preference for createServerMiddleware() without s since it's expected that a middleware may be composed of multiple middlewares. (And it's actually technically speaking accurate: the returned value is a single middleware function (req, res, next) (that uses several middleware functions).)

patak-dev and others added 3 commits June 17, 2022 14:31
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
docs/config/server-options.md Outdated Show resolved Hide resolved
docs/guide/ssr.md Outdated Show resolved Hide resolved
docs/guide/ssr.md Show resolved Hide resolved
patak-dev and others added 3 commits June 18, 2022 15:14
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
@patak-dev patak-dev merged commit 14db473 into main Jun 18, 2022
@patak-dev patak-dev deleted the feat/app-type branch June 18, 2022 13:29
danielroe added a commit to nuxt/framework that referenced this pull request Jun 20, 2022
danielroe added a commit to nuxt/framework that referenced this pull request Jun 20, 2022
danielroe added a commit to nuxt/framework that referenced this pull request Jun 23, 2022
danielroe added a commit to nuxt/framework that referenced this pull request Jun 28, 2022
danielroe added a commit to nuxt/framework that referenced this pull request Jul 4, 2022
danielroe added a commit to nuxt/framework that referenced this pull request Jul 6, 2022
danielroe added a commit to nuxt/framework that referenced this pull request Jul 8, 2022
danielroe added a commit to nuxt/framework that referenced this pull request Jul 8, 2022
danielroe added a commit to nuxt/framework that referenced this pull request Jul 12, 2022
danielroe added a commit to nuxt/framework that referenced this pull request Jul 13, 2022
@brillout brillout mentioned this pull request Aug 26, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change 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.

None yet

6 participants