feat: resolved createEnvironment #17791
Conversation
|
|
packages/vite/src/node/config.ts
Outdated
| function createDefaultDevEnvironment( | ||
| name: string, | ||
| config: ResolvedConfig, | ||
| context: CreateDevEnvironmentContext, | ||
| ): DevEnvironment { | ||
| return config.environments[name].consumer === 'client' | ||
| ? createDefaultClientDevEnvironment(name, config, context) | ||
| : createDefaultSsrDevEnvironment(name, config, context) | ||
| } |
There was a problem hiding this comment.
Before this PR, we were creating a client-like environment for any custom named environment that didn't define a createEnvironment function. I think this is a better default given that custom named environments are SSR-like by default.
@sheremet-va how do you see the default client environment if we deprecate ws? We can't have a createClientHotChannel() as we will need the HttpServer instance, no? If we want to avoid passing the whole Vite server, maybe we'll need to pass more things in the CreateDevEnvironmentContext.
I think moving this to the config face forces us to think what is needed to create an environment and play with the same rules for the default and custom ones.
There was a problem hiding this comment.
I'm sorry, I don't understand the question. We are deprecating the ws on the server, we are not removing the websocket server.
There was a problem hiding this comment.
So we need the client environment.hot to be this websocket server, no? The question is how do we create hot to pass to the client once server.ws is gone. Maybe the answer is that we still pass ws to CreateDevEnvironmentContext?
There was a problem hiding this comment.
Isn't it what I proposed? Pass down ws? #17791 (comment)
There was a problem hiding this comment.
Yes, this is what we are doing now. But you also said "But make it optional so we don't need to deprecate it when we add ws: false)". IIUC, it is ok to have it always there, it won't need to be made optional as we still need to pass the ws so the client environment can use it.
docs/guide/api-vite-environment.md
Outdated
| runner: { | ||
| transport: new RemoteEnvironmentTransport({ | ||
| send: (data) => worker.postMessage(data), | ||
| onMessage: (listener) => worker.on('message', listener), | ||
| }), | ||
| }, | ||
| watcher: context.watcher |
There was a problem hiding this comment.
For now, I moved to watcher: false | FSWatcher to force the user to pass the watcher forward. I tried doing this during init() but it doesn't feel right. Maybe it is ok for createEnvironment to be a bit more complex to implement given that only framework and runtime maintainers will be doing this?
There was a problem hiding this comment.
The watcher is only used in transformRequest. Maybe we can find a way to remove it from there somehow so it's not required in the environment at all?
There was a problem hiding this comment.
I just re-checked and we also need the watcher to implement the plugin context addWatchFile(). And I found a bug, as we weren't passing the watcher when creating the plugin container for each environment. I don't think we can remove this. I moved it to environment.init({ watcher }) at 569f714. We can decide if we want environment.watcher or we leave it as internal as it is used now environment.pluginContainer.watcher. Does this look better?
Description
Environment options
dev.createEnvironmentandbuild.createEnvironmentwere not resolved in the config stage. They remained undefined, and later the default was set in the builder and server while creating the environments. This PR requires both in theResolvedConfig, and forces us to properly define their type.dev.createEnvironmentwas not taking the thirdsetupparameter (so the watcher was not received by custom environments or for theclientandssrenvs when defining a customcreateEnvironment).Now that is exposed, we may need a better name than
setupfor this parameter.