feat: environment in hooks, context vs param

[patak-dev]

Description

This PR implements a change discussed with @antfu and @sheremet-va to the way Plugin hooks can access the current Environment. For reference, current implementation and docs for the feature:

• feat: environment api #16129
• docs: environment API proposal #16089

Context

Plugin hooks have always been able to know the current environment by the ssr flag passed to resolveId, load and transform hooks. We control dev, so we can pass it directly, and during build, we inject it in rollup by wrapping plugin hooks.

Plugin hooks use this boolean flag to access environment specific configuration (for example config.ssr.resolve.conditions). To access the deps optimizer associated with the environment (through a weak map against the resolved config getDepsOptimizer(config, ssr), but the optimizer has a internal dep against the server). To trigger processing of imports for warmup (using server.warmupRequest(url, { ssr })). Between other things, where most of them aren't specific to SSR but are conditions due to the need of separating requests from the client and SSR (as in, any other environment will need to do the same, even if not used for rendering in the server).

For the Environment API, initially we thought of following the same pattern deprecating the ssr boolean flag and replacing it with a environment string options param to hooks. When we implemented this version, it become clear that this forced a coupling of plugins against the whole server. If a plugin needed to access environment.config.resolve.conditons, then it would have to store the server in configureServer and call server.environments[environment] in the hook. Same for environment.depsOptimizer, environment.warmupRequest(url), etc. We have then an opportunity to reduce the coupling of hooks from the whole server to only the current environment for most plugins. So instead of passing a environment string, we modified the proposal to directly pass the environment instance. This move simplified a lot of code, and made it a lot more clear that resolveId, load, and transform are running against a specific environment (and that the plugin can completely ignore other environments and the server in most cases).

Environment instance in hooks: param vs context

We discussed a bit in the last Vite team meeting about how passing the environment instance to hooks could work in Rolldown. I think that for Rolldown, having the environment inside hooks is positive if we want to port vite internal hooks to Rust in the future. If we can share the environment state, then both Rust and JS can access it.

Passing the environment instance as a param to hooks doesn't feel right though. This PR moves the current environment to the PluginContext instead. This is more aligned with the way Rollup is designed. So, instead of:

transform(code, id, { environment }) {
  log(environment?.config)
}

Plugins would do:

transform(code, id) {
  log(this.environment?.config)
}

@sapphi-red @hyf0 opening a PR so we can discuss what is the best move here, so we don't introduce a change that will later on make it difficult to refactor the code to use Rolldown.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[sapphi-red]

so we don't introduce a change that will later on make it difficult to refactor the code to use Rolldown.

AFAIK it doesn't matter for rolldown whether the environment is passed from a param or a context.

This is more aligned with the way Rollup is designed.

I felt the param one to be more aligned with rollup. Rollup has options param in renderChunks hook. I felt environment is similar to that. Would you explain why you felt the context one more aligned?

[patak-dev]

This is more aligned with the way Rollup is designed.

I felt the param one to be more aligned with rollup. Rollup has options param in renderChunks hook. I felt environment is similar to that. Would you explain why you felt the context one more aligned?

In renderChunk, the options are general configuration of Rollup, no?

I'm thinking on environment more closely to this.getModuleIds(), that is also part of the PluginContext. We have this.environment.moduleGraph.getModuleById(id) is equivalent to it. Other examples: this.environment.depsOptimizer.isOptimizedDep(id), this.environment.transformRequest(url) is similar to this.load(id) in the context too. I think I'm starting to think of environment as Vite's extension of Rollup context instead of extra options some hooks receive.

[sapphi-red]

In renderChunk, the options are general configuration of Rollup, no?

It's outputOptions, which depends on the output format. I felt environment as an option that depends on the environment.

But I was convinced. It definitely is similar to this.getModuleIds().

[patak-dev]

Thanks for checking this out @sapphi-red! Let's merge it so we can keep iterating. We can keep discussing here and later revert if necessary.

About rolldown, I think we will end up finding a good way to share the environment. Two thoughts here:

• Maybe all functions should be async (like making this.environment.moduleGraph.getModuleById(id) async) to make things easier later. I would prefer we don't do this though as the API will be a lot more difficult to use forcing every hook to be async. We can discuss about it though if the Rolldown team thinks this is important. But even if we do this, we will still have the same problem with other sync functions in the context anyways.
• We currently pass the server to each Environment. Whatever we access from it, that would be good that is async too. Or that we try to only pass what is needed (like pluginContainer). I think it will be more clear as we keep cleaning up and moving things to the environment.

[sapphi-red]

In my strategy of parallelizing JS plugins, calling a main thread function from workers synchronously seems to work (sapphi-red/parallel-js-plugin-experiment#4). I'm not sure if it's safe to do it though. 😅But there's at least some hope. /cc @hyf0

For rust plugins and non-parallel JS plugins, sync things should be fine.

[hyf0]

calling a main thread function from workers synchronously

Yes. It's true if using napi-rs to wrap the function to syncify it. But I'm not certainly sure if this solution works well(I knew it works on some cases.

We used to use this solution to make const sync_require = napi_syncify((...) => await import('...'))and it doesn't works on this case. There may be something special, cause it's related to the module system. So I would consider things like napi_syncifynot reliable.

[patak-dev]

Even if we release sync APIs as part of this.environment in Vite 5.3 (in around two months), we could still later move them to async if it is really needed in Vite 6 before making the API stable (in around 6 months). Hopefully we will know by then if sync functions in the context are safe to use. If not, we would have a lot of ecosystem churn as we will also need to create async equivalents to every Rollup context function. So it is worth trying to avoid that.