feat: buildApp hook

[patak-dev]

Description

Implement a buildApp hook, with a similar API to @dario-piotrowicz' proposal:

• Environments build API #19714

I didn't end up going forward with the builder.build(environment) returning a promise proposal because we have a current use case for building environments multiple times with RSC right now (that hopefully won't be needed in the future).

Instead of a builtEnvironments array, this PR implements the same idea with a environment.isBuilt flag. We may end up adding more state later and I think we shouldn't create arrays for each of them. It feels a bit strange to have this state in the BuildEnvironment at first, but we have a lot of state in dev environments already.

Note: I ended up not removing config.builder.buildApp because it is easy to keep working and I think it would help with the current explorations to avoid the breaking change. We can still decide to do a clean move to a hook only world in Vite 7 or delay that to Vite 8 (if the hook proves to be the final shape of the API, what I'm leaning towards). We can deprecate config.builder.buildApp at one point during Vite 7 then.

[dario-piotrowicz]

This is really awesome! thanks @patak-dev! 🫶
(PS: wow this is also so much simpler than what I expected it to be! 😅)

[hi-ogawa]

Looks good to me 👍
For RSC, we are also thinking about generalizing scan build concept (for example, @jacob-ebey showed me this https://github.com/jacob-ebey/enhanceable/blob/ae92302978dc347d8e98fc533ed1c16edc53a1b2/src/plugins/scan-pass.ts#L21-L42) and I think buildApp plugin hook is an interesting direction to explore.

[sapphi-red]

Other than the @experimental jsdoc, it looks good to me 👍

[patak-dev]

The default build was still kicking in if no config.builder.buildApp was defined, building all environments. Fixed that in dcea0b1

I made config.builder.buildApp be a noop by default, and then decide if we should fallback in case there wasn't any environment built.

The other option would be to only build all environments when no buildApp hook or config.builder.buildApp is defined (and maybe we need to move config.builder.buildApp after all hooks in that case).

I don't know which of these two options is the best way to define the default behavior.

[hi-ogawa]

Instead of dcea0b1, can the default behavior adjusted by checking environment.isBuilt for each environment here?

if (!environment.isBuilt) {
  await builder.build(environment)
}

I think the difference is whether plugin.buildApp partially building environments should cause Vite to build the rest of environments. As a default behavior, this seemed natural to me. plugin.buildApp can still technically avoid that by doing delete builder.environments["noNeedThisEnv"] 🤔

[patak-dev]

We can't do that except if we move the order post hooks before config.builder.buildApp, and we also may deprecate and remove config.builder.buildApp in the future

[hi-ogawa]

Putting config.builder.buildApp aside #19971 (comment), I think my question is whether there's any rational in choosing the default behavior here "fallback to building all environments if no environments have been built" or the below, assuming config.builder.buildApp is removed:

// always build rest of unbuilt environments
for (const environment of Object.values(builder.environments)) {
  if (!environment.isBuilt) {
    await builder.build(environment)
  }
}

I don't see anything significant right now, so either way seems fine to start with, but I just wanted to check if I'm missing something.

[hi-ogawa]

Well, actually this seems like it's also about enforce: "post" hook (sorry, I should've just commented above). What's the expected behavior when there's post plugin.buildApp hook but without user's config.builder.buildApp now (and also hypothetically builder.buildApp is removed)?

[patak-dev]

The idea of the post hook was to give a way to plugins to build environments only if no other plugin has built them. See the problem described in the linked discussion. And they also want to make sure to do build related tasks after all environments have been built.

Once builder.buildApp is removed, the expected behavior with post is just to ensure it is run after all pre and normal order buildApp hooks. It is like having an afterBuildApp hook, but where you can still build environments. We could still have a beforeBuildApp and afterBuildApp hook.

But to the point in your other thread, I see now that it isn't only about order and the builder.buildApp hook. We can still move it as the last thing, after all post hooks (anyways, it is going to be removed). And to define the default as you propose building all environments that haven't been defined. In that case, the buildApp hooks will act more like a way to order the environment builds, more than deciding what gets built.

We could have an environment.build.manual: true config or something similar to define that this environment shouldn't be built if a hook hasn't build it.

[hi-ogawa]

The idea of the post hook was to give a way to plugins to build environments only if no other plugin has built them

Oh right, I remembered that's the whole point of providing isBuilt to user land. What I had in mind was "hook after all writeBundle" #16358 (comment).

[jamesopstad]

I think a really nice addition to this would be if the built environments include manifest objects. These would be the same format as you currently get by outputting the manifest file but would be stored in memory. Currently, frameworks often output a manifest file and then read it in order to move assets emitted in the SSR environment to the client bundle. Having this available on the environment (similar to isBuilt) would remove the need to read and write files.

I realise that this is probably outside the scope of this PR but bringing it up now as including additional data on the BuildEnvironment might affect the way you want to include environment.isBuilt. I can also imagine other use cases for adding additional data to the BuildEnviroment at build time, such as providing metadata that can be read elsewhere e.g. builder.build(environment, meta).

[patak-dev]

How to keep the manifests around is an interesting point. Maybe we could have it as environment.manifest, that doesn't strictly invalidate the need for environment.isBuilt if we keep letting projects opt-in into manifest collection. This would only be needed if the manifest of an environment needs to be accessed by different buildApp hooks.

Next we may want an opt-in environment.bundle, more if we allow for builder.createBundle(environment) (equivalent to rollup.rollup()). I'm a bit unease about this API though, of adding all these artifacts at the top level of environment. It may be ok if these are the only props we add. Having an environment.result also feel a bit strange to me.

[jamesopstad]

I created a new issue for the manifest feature discussed above - #20052.