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: remove builder.entireApp #18028

Closed
wants to merge 2 commits into from
Closed

fix: remove builder.entireApp #18028

wants to merge 2 commits into from

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Sep 4, 2024

Description

The idea I had for #16471 (comment). Feel free to let me know too if this is a bad idea

What I'm thinking is that build.entireApp currently has a different meaning:

  1. If set by the user or plugins, the CLI is able to dynamically call build() or createBuilder()
  2. But from a programmatic API pov, frameworks can't do the same as they need to decide early on which API to use. Perhaps the framework could support both depending on whether the user sets the config, but they can't emulate the behaviour like the CLI.

With this PR, I removed the build.entireApp option entirely and let frameworks manage them themselves.

  1. If they only support --app, they should call createBuilder or ensure the vite CLI is called with --app
  2. If they only support the normal build API, they should call build() (same as before, not changed)
  3. If they support both, users can choose via --app, or frameworks can expose an option to pick between both.

Copy link

stackblitz bot commented Sep 4, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@patak-dev
Copy link
Member

Let's discuss about this one after we merge the PR. We can point to main after that.

Base automatically changed from v6/environment-api to main September 4, 2024 09:27
@sapphi-red
Copy link
Member

I like this.

  1. ... ensure the vite CLI is called with --app

I have one question about this one. Is it easily possible to detect whether it was called with --app?

@sapphi-red sapphi-red added the feat: environment API Vite Environment API label Oct 3, 2024
@bluwy
Copy link
Member Author

bluwy commented Oct 3, 2024

I guess you could check process.argv.includes('--app') to check that.

sapphi-red
sapphi-red previously approved these changes Oct 4, 2024
@sapphi-red
Copy link
Member

@patak-dev Do you have any thoughts here?

patak-dev
patak-dev previously approved these changes Oct 21, 2024
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.

I think we should give frameworks a way to have a clean vite build in the future. Once they start using buildApp(), it probably doesn't make sense to use vite build to only build the client anymore (there may be other steps between environments that are needed, the client build may need info from other environment builds, etc). Having every app needing to add --app could lead to confusion and hard to debug errors (we could add a warning if there is a buildApp() though). For frameworks like SvelteKit, it would also count as a DX regression. They are currently able to make vite build generate the whole app (through the writeBundle hook hack). Once they move the environments, they will need to update every package.json build script to be vite build --app. I think they won't accept this to be honest, and it may be a blocker for them.

Having said that, I'm approving the PR, because if you'd like to release Vite 6 without entireApp, we can always add this later, maybe with a different option or using buildApp() as a way to opt-in.

@bluwy bluwy dismissed stale reviews from patak-dev and sapphi-red via e8e4141 October 21, 2024 15:21
@bluwy
Copy link
Member Author

bluwy commented Oct 21, 2024

I personally don't think it should be a blocker since SvelteKit often has a migration script to handle that. And I think --app being a visual cue is nice and implies it's building the entire thing, which vite build alone wasn't meant to have worked before.

I'll ping them though if they have thoughts about this, but I also agree that if we add this later on it can be a feature.

@sapphi-red sapphi-red added this to the 6.0 milestone Oct 22, 2024
@dominikg
Copy link
Contributor

suddenly requiring to add --app to what previously was just vite build seems like an arbitrary breaking change for not much benefit.

If at all i'd say let vite build run everything and vite build --environment=xxx can pick a subset to build, could be a repeatable key to allow picking 2 out of 5. Might need topological sorting if one env depends on another tho

@bluwy
Copy link
Member Author

bluwy commented Oct 22, 2024

It's not required to add --app unless you want Vite to build the environments for you, and you'd have to restructure the build process slightly to opt-in in the first place. And I think the distinction here upholds our intent that vite build was only meant to do a single build, which I think is valuable to have.

IIUC the builder API is also experimental (I heard it was but we don't mark it as experimental 🤔), so pushing for vite build as "build all environments" feels too early. Not to mention it'll break many existing Vite setups v6 and below.

@patak-dev
Copy link
Member

My take here is that we will still need something like what we are removing in this PR, or SvelteKit will keep using the writeBundle hack and ignore buildApp. This API was specifically targeted at SvelteKit, so I'll push for vite build to build the entire app in the future.

@bluwy
Copy link
Member Author

bluwy commented Oct 22, 2024

For me, if we do want vite build to build an app based on a config option, as long as it can be replicated with the build() JS API (without needing to decide beforehand either createBuilder() and build() because that defeats the config option purpose), I'd be fine with it.

The CLI and JS API symmetry feels more important to me, than whether --app is needed or as a default or not.

@patak-dev
Copy link
Member

We decided to go with #18432, that would allow SvelteKit and others to keep using vite build if they want to experiment with the new buildApp API instead of the writeBundle hack

@patak-dev patak-dev closed this Oct 23, 2024
@bluwy bluwy deleted the build-app-cli branch October 23, 2024 13:52
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.

4 participants