feat: extended applyToEnvironment and perEnvironmentPlugin

[patak-dev]

Description

Start from this discussion for reference.

We need to allow users to use plugins that aren't prepared to be run concurrently for multiple environments (many rollup build plugins for example, that keep cross hooks state). This was easy with some of the previous iterations of the Environment API (for example when we could pass functional plugins, or when we had the environmentPlugins hook that added plugins after the current one).

This PR proposes an extension to the current design that I think feels more integrated. The applyToEnvironment hook currently only returns a boolean to decide if a plugin is active or not in an environment. The idea is to also allow return PluginOption from it, in which case, this plugins are used instead.

applyToEnvironment: (environment) => boolean | Promise<boolean> | PluginOption

We could make the PluginOption more strict, but I don't know if it is worthy.
The returned plugins are inserted replacing the current plugin. enforce will be ignored. If a user would like to rearrange them, they can use per hook order or a enforce in the plugin that has the applyToEnvironment.
We also ignore the config, configEnvironment, configureServer, configResolved hooks because these have already been called when applyToEnvironment is run.
We also ignore applyToEnvironment in these.

I took the opportunity to also allow this hook to be async.

There is also a new perEnvironmentPlugin helper (the makeEnvironmentAwarePlugin that @sapphi-red proposed).

With this PR, we can also remove two TODOs and support per-environment build.commonjsOptions and build.rollupOptions.plugins.

I renamed usePerEnvironmentState to perEnvironmentState and also expose it.

[patak-dev]

One thing to note about this design is that applyToEnvironment is resolved after the environment instance is created. This makes it easier to use the resolved environment.config to configure the per-environment plugins.

As I said in the description, with this approach, you can't return any kind of plugin from applyToEnvironment. The config, configEnvironment, configureServer, configResolved hooks won't be called. All rollup plugins would be ok, but a lot of Vite plugins that aren't environment aware won't work when used with this function.

If we want to support any non-environment-aware vite plugin, that could be done by adding several of them in the root plugins, each with applyToEnvironment(env) { return env.name === env_name }. This is easy to do if an app has two or three known environments. But doing it with a helper in a generic way is tricky. The problem is that we need to first call config in all plugins to know all environments. Maybe we could do that, then we can call a perEnvironment hook that returns a PluginOption:

  plugins: [
    {
      name: 'per-environment-plugin',
      perEnvironment(environment: PartialEnvironment) {
        // use environment.config if needed
        return nonShareableVitePlugin()
      }
    }
  ]

These could get config called after all other plugins (because we first need to know the names of all environments), and could get configEnvironment, and configResolved (configureServer also?).

We have two options to implement this:

1. All these plugins will be added to the main pipeline with the correct applyToEnvironment that returns true only for the correct environment.
2. We refactor the environment plugins init so it happens right after config has been called for all plugins, instead of doing it after creating the Environment instance.

Actually, if we do 2, then this hook could be directly applyToEnvironment, taking a PartialEnvironment and that would mean that we can have the API in this PR, but also be able to use it for non-environment aware vite plugins.

I'll make this PR a draft and try to do the refactoring so we can discuss with something concrete in the table.

[patak-dev]

We refactor the environment plugins init so it happens right after config has been called for all plugins, instead of doing it after creating the Environment instance.

I tried to implement this, but it doesn't work. If we call config and configEnvironment hooks in the plugins returned by applyToEnvironment(), then there are edge cases all around. We are using applyToEnvironment: environment => environment.config.xxx in some of our internal plugins for example. These conditions start depending on where the plugin is in the pipeline if the newly added plugins can further change the configuration. And if we call these hooks after all others, then the changed config will not have an effect over applyToEnvironment().

This is similar to why we never wanted to allow plugins to return other plugins in the config phase in a sense. So I think we should have clean phases:

config -> configEnvironment -> applyToEnvironment -> configResolved

applyToEnvironment receives the resolved config, and can decide what plugins to return based on the final configuration. Plugins returned from applyToEnvironment won't be able to re-configure the app. These plugins should get their configResolved hook called though, and also configureServer.

Ideally, we should move applyToEnvironment to the config resolution phase and change the signature to take a PartialEnvironment. There is a problem with doing this right now if we don't use a shared config during build. To do it cleanly, we would end up resolving the plugins for all environments every time that we resolve the config (and during build, this happens once per environment). So we may need to accept that applyToEnvironment is called at Environment init to avoid this for now. We could already change the signature to be PartialEnvironment so we can later on refactor things to be clean if shared config is the default at one point. I think we can move forward with this PR, or the discussions related to it, and do these changes in other PRs.

[bluwy]

Taking a step back, does it make sense to ditch extending applyToEnvironment, and introduce perEnvironmentPlugin only instead? Or a step further, keeping things as is and only introducing supporting functions in plugins?

For example:

export default defineConfig({
  plugins: [
    // create a new plugin per environment. do we need to pass the environment name?
    // i think they can already retrieve that information from `this.environment`?
    // these kinds of plugins likely wouldn't care about environments if needed. if they did,
    // they could directly support shared instance in environments in vite instead
    () => nonSharedPlugin()
  ]
})

I think we've discussed this before in the past, and maybe it is the easier way to solve this problem. Maybe only enabling this behaviour if sharedConfigBuild before processing plugins.

If we don't want to plainly support functions (which could be (ab)use by third-party plugins), we could have perEnvironmentPlugin(() => nonSharedPlugin()) that labels it so it only works with that API.

[patak-dev]

I really like the idea of having functional plugins. They are the most ergonomic way to do per-environment plugins, by a fair margin. We implemented it and later on decided to remove because:

• It could introduce breaking changes. There are plugins that are filtering config.plugins and expect all of them to be an object. Maybe we could accept these though and ask the ecosystem to adapt.
• Functions don't let us add modifiers. For example, we can no longer have a name, the enforce, apply, etc, or later on other per-plugin option flags like the ones we have added in Vite 6. When we implemented it, we actually allowed these modifiers, by letting the user add the props to the function. The types work, and it is an option. We could also decide that enforce is no longer needed because we have order in hooks now.
• I think the most important reason was that we didn't want to make it so easy to have per-environment plugins. We wanted to try to keep all plugins being shared in a unified pipeline. This PR changes that... so maybe you have a good point that we should re-evaluate.

I can work on a PR to see how functional plugins would look like with the current state of Vite 6. We would need to keep applyToEnvironment() => boolean too.

[patak-dev]

Ah, if we want to be able to use these plugins internally, that would be a breaking change as everybody will see functional plugins in their pipeline (for example if we want to add environment => environment.config.build.rollupOptions.plugins). If we go with functional plugins, one option would be to do a similar trick to what @sapphi-red did in rolldown/vite and have a internal-only way to add an object plugin that works like a functional plugin.

[patak-dev]

@bluwy I started implementing functional plugins, but I think it is easier to just check the implementation change from functional to environmentPlugins here:

• feat: back to environmentPlugins hook #16732

I don't think we should go with functional plugins. If you check the PR, the types for PluginOption become more difficult and working with the array more cumbersome. But above this (and the potential extra breaking changes), we would be adding an easy way to do something we don't want people to do. I think we should go with the extended applyToEnvironment as proposed in this PR for Vite 6.

[patak-dev]

Pushed refactor: use PartialEnvironment in applyToEnvironmnet, so even without a refactoring, we are opening the door to move the applyToEnvironment hook call during the config resolution phase. The idea is that users shouldn't rely on the final Environment instance (or modify it, etc)

Also after feat: call configResolved hook on new plugins, the plugins returned from applyToEnvironment will get their configResolved hook called. I think this is important as it is a very common pattern to use this hook to cache the config (pre this.environment era).

I'm not sure if we should add support for this plugins to use configureServer. I think we can wait until someone gives us a good use case for that.

[bluwy]

After further thought about this, I think your implementation still seem to be the best middleground. However, personally, I wouldn't support configResolved. Maybe we should highlight which hooks do not run for now, and then have users requesting certain hook support later if they need it?

I also think still that we should probably avoid supporting returning plugins from applyToEnvironment, at least hide it from the types. It feels to me that it's plugin author facing (which they should've refactored to be environment aware first), and it could be abused to return plugins unrelated to environments purposes.

[patak-dev]

However, personally, I wouldn't support configResolved. Maybe we should highlight which hooks do not run for now, and then have users requesting certain hook support later if they need it?

I think we need this hook. I agree with the sentiment (regarding other hooks like configureServer), but configResolved is the way to access configuration from plugins. I think we need to support this one from the start. The hook is experimental so we can later change it if we discover issues. For example, one possible problem could be if the same plugin is returned for several environments but I don't see why someone would do that.

I also think still that we should probably avoid supporting returning plugins from applyToEnvironment, at least hide it from the types. It feels to me that it's plugin author facing (which they should've refactored to be environment aware first), and it could be abused to return plugins unrelated to environments purposes.

The current applyToEnvironment() => boolean is also intended for final users as much as plugin authors. The idea is that they would add it as enforce:

{ ...plugin, applyToEnvironment(environment) => environment.config.foo }

So I think adding PluginOption changes the equation here. If we want to simplify this, maybe we could change the hook to just be swap(environment) => PluginOption. PluginOption also allows for falsy values, so maybe this is a simpler definition and nicer to use:

{ ...plugin, branch(environment) => environment.config.foo }

and

{
  name: 'per-environment-rollup-plugins',
  branch(environment) => environment.build.rollupOptions.plugins
}

Or other names like spread, swap, switch, substitute (rollup hooks are normally a single word, applyToEnvironment(environment) => environment... is a mouthful so I would be happy to find a better name).

[sapphi-red]

I wouldn't support configResolved personally. I thought perEnvironmentPlugin was to give users a escape hatch to use (non environment-aware) rollup plugins. If the plugin author is happy to make the plugin Vite-friendly (like using configResolved hook and tweaking the behavior), then I think the plugin should be environment-aware.

[patak-dev]

Ok, I reverted the last commit, so configResolved isn't called now. I still think this is useful, but we can always add it later once we have more concrete use cases to justify it.

[bluwy]

So I think adding PluginOption changes the equation here. If we want to simplify this, maybe we could change the hook to just be swap(environment) => PluginOption.

I'm thinking that we should disallow any way for plugins to add plugins in the first place, or at least not make it easy to do so. Regardless if it's exposed via applyToEnvironment or branch, I was thinking like what sapphi mentioned above, the API is only an escape hatch for non-env-aware-cache plugins, that would usually be applied from the end-user side.

If we make it part of the hooks, it makes it easy to use the escape hatch when instead what they should really do is to use the perEnvironmentState utility or refactor in some way to not tie to that. In any ways, that plugin library or its users could always use the perEnvironmentPlugin utility to patch things while the proper work is being done.

To be clear in case it got confusing, I think applyToEnvironment: () => boolean and perEnvironmentPlugin() makes sense, but not applyToEnvironment: () => PluginOptions for me. We could support that as an implementation detail, but not as public API.

[patak-dev]

Would you like to discuss this in the next Vite team meeting? If not, I think we could move forward with it

[bluwy]

I'm ok with merging if we remove the types for returning plugins in applyToEnvironment, otherwise we can discuss this in the next team meeting or chat.

[patak-dev]

I think we shouldn't have only the helper function. We should follow the standard way hooks work in Rollup. We can discuss tomorrow then.

[patak-dev]

We discussed in today's team meeting and agreed on merging this PR. Some notes:

• @bluwy raised concerns about potential edge cases introduced by applyToEnvironment returning plugins
• he was fine adding the perEnvironmentPlugin helper but wanted to hide the feature in the hook. We decided that the hook and helper are introducing the same functionality and they can be abused in the same way by users and plugin authors. So we'd like to move forward with the hook (and adding the helper as syntax sugar, and not as a new way to toggle behavior in plugins).
• this feature is experimental (as all others in environment API), and it is a good idea to include it in v6, so the ecosystem can explore how to best do environment aware plugins.

Merging, and I'll send another PR later to discuss a possible name change for the hook.