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

refactor: use builder in build #18432

Merged
merged 5 commits into from
Oct 23, 2024

Conversation

sapphi-red
Copy link
Member

Description

Alternative to #18028

  • Removes builder.entireApp and use builder: {} / builder: undefined (this one can be discussed separately if needed)
  • Introduce a internal concept of legacy builder that just works like build and use that from the CLI

This makes the createBuilder a bit complicated but the overall code size is smaller and improves CLI-JS API symmetry.

Migration Plans

This PR does not change any behavior. Later on we make builder: { sharedConfigBuild: true } to be the default and remove the codes related to the legacy builder.

  • For CSR only apps: no migration needed
  • For CSR + SSR apps using vite CLI
    1. No change is needed for Vite 6, but can opt-in to using builder by passing --app or setting builder: {} in the config
    2. When the legacy builder is removed, users can remove --app but that change is not necessary (we should just ignore the flag)
  • For CSR + SSR apps using build command
    1. No change is needed for Vite 6, but can opt-in to using builder by using createBuilder
    2. When the legacy builder is removed, users need to migrate to createBuilder from build

@sapphi-red
Copy link
Member Author

@patak-dev @bluwy What do you think about this one?

@sapphi-red sapphi-red added the feat: environment API Vite Environment API label Oct 23, 2024
Comment on lines 1548 to 1549
if (selectedEnvironmentName && selectedEnvironmentName !== environmentName)
continue
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this would be more clear with a condition out of the for loop:

  function setupEnvironment(environmentName, environmentConfig) {
    const environment = await environmentConfig.build.createEnvironment(
      environmentName,
      environmentConfig,
    )
    await environment.init()
    environments[environmentName] = environment
  }

  if (selectedEnvironmentName) {
    setupEnvironment(selectedEnvironmentName, config)
  }
  else {
    // ... current for loop using setupEnvironment(environmentName, environmentConfig)
  }

@patak-dev
Copy link
Member

This approach looks good to me! Thanks Sapphi for exploring this option.

I was thinking we could move some props later on from build to builder, for example copyPublicDir, but we can delay this until everyone is using buildApp. This, thinking in a future where nobody wants to use Vite to build a single environment (and if they want, they just configure it with a single environment, but there is no more build() a part).
If we don't think that will ever be possible, maybe we could use { builder: { entireApp: true }} for the option, but using the implementation proposed here.

@bluwy
Copy link
Member

bluwy commented Oct 23, 2024

I like this approach too, so IIUC the programmatic API to reflect vite build and vite build --app would be createBuilder({}, null).buildApp()? It's a little clunky, but I think it's fine since it's only a stop gap for this major.

Once createBuilder() stabilizes, would we phase out build()?

@sapphi-red
Copy link
Member Author

so IIUC the programmatic API to reflect vite build and vite build --app would be createBuilder({}, null).buildApp()?

Yes.

Once createBuilder() stabilizes, would we phase out build()?

I think yes.

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

Thanks again Sapphi! I prefer this option to just removing entireApp. Just a note again that if we go with this one, we are already deciding that at one point we would like to remove build() and being able to "build a part of an app". Or we can still change our mind and accept a small breaking change for people that used builder: {} if we need to introduce entireApp. I'm fine with this.

@patak-dev patak-dev merged commit cc61d16 into vitejs:main Oct 23, 2024
14 checks passed
@sapphi-red sapphi-red deleted the refactor/use-builder-in-build branch October 24, 2024 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: environment API Vite Environment API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants