docs: environment API proposal #16089
Conversation
|
|
patak-dev
changed the title
patak-dev
changed the title
patak-dev
added
feat: environment API
docs/guide/api-vite-environment.md
Outdated
| ### Accessing the current environment in hooks | ||
|
|
||
| The `environment` name is passed as a parameter to the `options` of `resolveId`, `load`, and `transform`. |
There was a problem hiding this comment.
If we want to conditionally return a virtual file for worker-like runtimes such as Cloudflare workerd and Fastly Compute, is there a way to do it at once?
I guess it is required to write it like:
const p = {
name: 'virtual',
load(id, opts) {
// it requires to write all the environment names
if (opts.environment === 'workerd' || opts.environment === 'fastly-compute') {
return 'a code for worker-like runtimes'
}
return 'a code'
}
}But to migrate from config.ssr.target === 'webworker', I think it'd nice to write it something like:
const p = {
name: 'virtual',
load(id, opts) {
const e = server.environment(environment)
if (e.type === 'worker') {
return 'a code for worker-like runtimes'
}
return 'a code'
}
}There was a problem hiding this comment.
This is a great point. We discussed two weeks ago a bit about this with @sheremet-va. In Vite core plugins, we have several conditions that depend on the ssr flag and other that depend on ssr.target: node | webworker as you explain (other ssr config options are already accounted for in the current state of the proposal).
If we want to remove the ssr flag, we need something like you propose here. I think we should move away from conditionals based on the environment name to conditionals based on what the environment supports. So, totally agree with you. I don't think it should be called type as it is too generic, but we need to identify what are the conditions not covered by resolve.conditions et al and add them as part of the environment config.
Maybe something like:
type runtimeType = 'browser' | 'node' | 'worker' | 'edge' | ?The problem with this is that there will be differences between edge environments and node environments. So it may be worth checking all conditionals in core and other projects and see if we can be even more fine-grained. Maybe it is a more generic option we should add to resolve for example.
If we end up using only fine-grained options in core, I still think that having a runtimeType is a good idea, even if it is only to help with debugging.
There was a problem hiding this comment.
Can you show an example when this is useful? I think having fine-grained config should be enough.
There was a problem hiding this comment.
I was thinking on the runtimeType also as a way for community plugins to be able to ship features before we add the proper options in Vite core. If we are missing a fine-grained config, it takes time for it to end up in a release. So it may be useful to know the runtime type to do some conditionals until they can switch to fine-grained options. I'm good avoiding this until we have a good use case to justify it.
There was a problem hiding this comment.
I assumed that we have many ssr.target conditions in our codebase, but it turns out those can be rewrote with a fine-grained option. Yeah, it seems we don't need something like this.
I was thinking of returning a virtual module conditionally, but I noticed that it can be achieved by using an exports field with wintercg condition.
| * Trigger the execution of a module using the associated module runner | ||
| * in the target runtime. | ||
| */ | ||
| run: ModuleRunFunction |
There was a problem hiding this comment.
What is the type of ModuleRunFunction? Is it (...args: any) => any or () => void?
There was a problem hiding this comment.
So far, the proposal assumed it to be (url: string) => void. But given your questions, and a discussion between @sheremet-va and @bluwy yesterday, I think it may be better for it to be (url: string, options?: any) => any and let each environment define the options and return type so code like you proposed here will work:
// the input and the output depends on the environment
const response = await server.environment('workerd').run(nodeReqToNativeReq(req))
const html = getResponseContent(response)There was a problem hiding this comment.
So far, the proposal assumed it to be (url: string) => void.
The proposal was (url, req?) => void
There was a problem hiding this comment.
I forgot we wanted to allow for passing the request too. I think it could go inside options for environments that would need that, no?
There was a problem hiding this comment.
Options are too environment-specific, we need a general interface that frameworks can just use without knowing what environment it is. I would expect most environments to throw an error if request is not passed
There was a problem hiding this comment.
we need a general interface that frameworks can just use without knowing what environment it is.
To achieve this, I guess we need to specify the return type. For example, limiting to a Response object.
There was a problem hiding this comment.
I don't think we can limit the return type to be honest. I would make it unknown so you have to check the return, but you can always call it with same parameters at least.
There was a problem hiding this comment.
I'm a bit confused about this too. It looks like this is the only interface for "environment implementer" (e.g. cloudflare workerd) to provide a unique feature to "environment users" (i.e. framework authors).
I feel this interface is somewhat analogical to what @sapphi-red's suggested to use plugin.api in
https://github.com/sapphi-red/vite-envs/blob/7f76892b7d28f0da06826f43953cedb5b2f042c5/packages/example-framework/src/index.ts#L16-L18C19
So, how about Vite to only define this interface with no constraint like this?
interface ModuleExecutionEnvironment {
api?: unkonwn
...
}some hypotetical code
Then, environment implementer side might provide:
// in vite-environment-workerd package
export interface WorkerdEnvironmentApi {
dispatchFetch(req: Request): Promise<Response>
}and framework author side would use it in a similar way as sapphi-red's plugin.api (or also Cloudflare team's POC, which is likely inspired by sapphi-red's api https://github.com/dario-piotrowicz/vite-runtime-5.1-experimentations/blob/2fa5ecead1fc60b30a4952f8bd7fdf4bf22aba4f/examples/example-framework/vite.config.js#L9-L15)
import type { WorkerdEnvironmentApi } from "vite-environment-workerd"
const frameworkPlugin = {
configureServer(server) {
const workerdEnv = viteServer.getEnvironment("workerd");
const workerdApi = workerdEnv.api as WorkerdEnvironmentApi;
return () => {
server.middlewares.use(async (req, res) => {
const webRes = await workerdApi.dispatchFetch(toWebRequest(req));
fromWebRequest(webRes, res)
}
}
}
}Ah, I totally ignored run(url: stirng) part, but from the yesterday's discussion, I feel there's always serialization issue and we cannot assume anything universally for all environments.
Also, for Workerd use case, entry file is probably assumed to be a part of environment initialization https://github.com/dario-piotrowicz/vite-runtime-5.1-experimentations/blob/2fa5ecead1fc60b30a4952f8bd7fdf4bf22aba4f/examples/example-framework/vite.config.js#L53, so run(url: string) might not make sense either.
There was a problem hiding this comment.
For reference, we are also discussing with @IgorMinar in this thread about run: #16089 (comment). Commented about the serialization part there.
Would you expand on the environment initialization issue?
I think how run (or something like your api) proposal will pan out is the biggest unknown to be defined right now in the proposal.
docs/guide/api-vite-environment.md
Outdated
| export default { | ||
| builder: { | ||
| runBuildTasks: asnyc (builder, buildTasks) => { | ||
| return Promise.all(buildTasks.map( task => task.run() )) |
There was a problem hiding this comment.
Are we going to reuse plugins? I guess that would have the same problem with #11218. (In dev, it won't be a big problem because buildStart hook will only be called once)
We can load the config for each build task, but that will make it difficult to share state among build tasks.
There was a problem hiding this comment.
Right now, the implementation resolves the config for each build generating a separate pipeline. I did it this way so we can parallelize without issues given the current state of plugins in the ecosystem.
@antfu proposed that we should take the opportunity and try to align with dev, and have a single plugins pipeline during build so it is easier to share information during build time too. We're going to push everyone to use something like this.environment.cache if they need to store things while processing (maybe we need a this.environment.api for comms?), if the plugin isn't environment agnostic. In dev, this isn't a stretch as people are already used to do the same with the ssr flag (probably there are a lot of bugs due to improper isolation because things aren't as clear as having a proper environment abstraction). During build, we could do the same and ask everyone to be aware of the environment that is being built. Running the plugins in parallel, even for workers (that we may rethink as environments too) becomes possible.
I'm worried about this idea though. I think it is an interesting goal to aim for, but a lot of plugins will be broken for vite build --all if we do this. I was proposing that we have a builder.isolatedPlugins flag that we make opt-in for a major or two and let users set it if all their plugins work well in this new world.
There was a problem hiding this comment.
About buildStart, we actually would like to go the other way around and call buildStart during dev once per environment and make plugins use that to reset the state of each environment. This could be very interesting if at one point we would like to implement a "restart this environment" if a config changes for example without the need for a full server restart.
There was a problem hiding this comment.
Is there a way for plugin to know the start or end of certain environment build?
For example, currently it doesn't look like environment is accessible from buildStart, buildEnd, writeBundle etc...
There was a problem hiding this comment.
We need to implement that, this.environment should be available for all hooks. So far we only injected it in resolveId, load, and transform as it was done before.
I wonder if Rollup could give us a better way to inject the environment so we don't need to wrap every hook, that feels quite inefficient
| While the `config` hook is running, the complete list of environments isn't yet known and the environments can be affected by both the default values from the root level environment config or explicitely through the `config.environments` record. | ||
| Plugins should set default values using the `config` hook. To configure each environment, they can use the new `configEnvironment` hook. This hook is called for each environment with its partially resolved config including resolution of final defaults. | ||
|
|
||
| ```ts | ||
| configEnvironment(name: string, config: EnvironmentConfig) { | ||
| if (name === 'rsc') { | ||
| config.resolve.conditions = // ... | ||
| ``` |
There was a problem hiding this comment.
Is it impossible to use configResolved hook?
There was a problem hiding this comment.
I don't think we should promote modifying the config in configResolved. When this hook is called, it should be safe to assume that the config is settled and plugins can read it and cache/store what they need. If we want to do this config in another hook, it should be in the config hook. And we could do that without a problem. The only reason why the configEnvironment hook exists is to help plugins deal with the complexity of environment overrides. I'm ok removing it if we think it is redundant, but at this point I feel that it will be very helpful to avoid subtle bugs in plugins.
There was a problem hiding this comment.
Is this configEnvironment only for custom environments or do you also want to move people to configure name = "client" and name = "ssr" cases here as well? For example,
sample code
// from this
config(config, env) {
return {
optimizeDeps: { ... },
ssr: { optimizeDeps: { ...} }
}
}
// to this?
configEnvironment(name, config) {
if (name === "client") {
config.dev.optimizeDeps = { ... }
}
if (name === "ssr") {
config.dev.optimizeDeps = { ... }
}
}
// or this?
config(config, env) {
config.environments.client.dev.optimizeDeps = { ... }
config.environments.ssr.dev.optimizeDeps = { ... }
}
// or this?
config(config, env) {
return {
environments: {
client: {
dev: {
optimizeDeps: { ... }
}
}
ssr: { ... }
}
}
}I thought it will be mostly redundant, but after writing this sample code, maybe configEnvironment looks explicit and nicer even for builtin ones. But maybe this assignment form of config needs to take care config mergining inside the hook itself?
There was a problem hiding this comment.
Ah, I forgot that ResolvedConfig is a readonly. 😅
At this point I feel that it will be very helpful to avoid subtle bugs in plugins.
What kind of bug do you have in mind?
There was a problem hiding this comment.
Is this
configEnvironmentonly for custom environments or do you also want to move people to configurename = "client"andname = "ssr"cases here as well?
Yes, configEnvironment will be called once per environment (including default ones). It should also work like config regarding being able to modify inplace or return an object to be merged (or we have a bug, or it isn't implemented yet)
There was a problem hiding this comment.
At this point I feel that it will be very helpful to avoid subtle bugs in plugins.
What kind of bug do you have in mind?
When you configure environments in the config hook you should first do things like: config.environments.custom ??= {}. You also have two ways to configure an environment:
- set it as a default affecting all environments
- set it explicitly for a given environment
If the default is already set, maybe it is better to avoid setting it explicitly? It may be a valid question. You also don't know from a plugin what are all the configured environments (because other plugins after you may add new ones), and maybe you want to do something with all of them.
In configEnvironment, all environments are already known, and defaults have already been applied. So you see a partially resolved environment options.
What I'm thinking we could suggest to plugin authors is:
- Add environments and set defaults in
config - Configure specific environments at
configEnvironment
We renamed EnvironmentConfig / environment.config to EnvironmentOptions / environment.options here c7e4da2
Rationale: I need to reach out for the root or some other global config from the environment. We currently can do that for a DevEnvironment with environment.server.config that is bad because you need a dependency on the server to get the config that doesn't depend on it. Currently there is no way to get the global config in a BuildEnvironment. Or I think you could do it through environment.builder.config that reads well but doesn't allow for generic dev/build code. If you only need the root, it would be better to avoid going through the server or builder. We also now have environment.config be a EnvironmentConfig. This also reads well, but given that we use const { config } = server in tons of places, it is very confusing now to see config and not know if it is the global config or the environment config. I think we should change environment.config to be the global ResolvedConfig. It still makes sense, this is the config for the environment, root is shared but is also part of the environment config. This will make config easier to understand everywhere.
I still think we need a way to have the EnvironmentConfig from the environment. So I'm thinking that I will rename it to EnvironmentOptions (aligning it with BuildOptions and DevOptions). And then you can access these as environment.options
We should also rename configEnvironment to environmentOptions aligning with options from rollup.
There was a problem hiding this comment.
But maybe this assignment form of config needs to take care config merging inside the hook itself?
In my comment above, I was worrying about how merging works in configEnvironment, but I just realized it can return partial options and merged automatically.
configEnvironment(name, _config, _env) {
if (name === "ssr") {
return {
build: {
rollupOptions: {
input: {
index: entry,
},
},
},
};
}
},
| While the `config` hook is running, the complete list of environments isn't yet known and the environments can be affected by both the default values from the root level environment config or explicitely through the `config.environments` record. | ||
| Plugins should set default values using the `config` hook. To configure each environment, they can use the new `configEnvironment` hook. This hook is called for each environment with its partially resolved config including resolution of final defaults. | ||
|
|
||
| ```ts | ||
| configEnvironment(name: string, config: EnvironmentConfig) { | ||
| if (name === 'rsc') { | ||
| config.resolve.conditions = // ... | ||
| ``` |
There was a problem hiding this comment.
Ah, I forgot that ResolvedConfig is a readonly. 😅
At this point I feel that it will be very helpful to avoid subtle bugs in plugins.
What kind of bug do you have in mind?
Co-authored-by: 翠 / green <[email protected]>
| ```ts | ||
| interface HotContext { | ||
| file: string | ||
| timestamp: number | ||
| modules: Array<ModuleNode> | ||
| read: () => string | Promise<string> | ||
| server: ViteDevServer | ||
| } | ||
| ``` | ||
|
|
||
| - `this.environment` is the module execution environment where a file update is currently being processed. |
There was a problem hiding this comment.
Currently it's implemented as HotContext.environment instead of this.environment.
patak-dev
changed the title
Co-authored-by: Clément <[email protected]>
Description
Important
The Environment API work has been consolidated into #16471
Vite Environment API proposal docs.
For the proposal implementation, see:
Deployed docs for this PR
This API was discussed between @sheremet-va, @antfu, and me, and validated with Pooya Parsa, Ryan Carniato, Evan You, Daniel Roe, and Dominik G (we'll ping them once this PR is ready to be reviewed). Feedback from all of them was positive, and they think this would unlock a lot of use cases in Nitro and the frameworks they work on. It was later on discussed and refined with other members of the Vite team and maintainers in the ecosystem.
This proposal merges the Vite Runtime API and the separate module graph proposal into a new unified API. Discussions were triggered by the experimentation done by sapphi-red, hi-ogawa, the Remix and Hydrogen teams, and the feedback received from the Cloudflare team.
Related PRs:
There is still a missing section about the new separate module graphs API, that I'll add later.