-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
feat: resolve environments plugins at config time #20120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: resolve environments plugins at config time #20120
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change sounds good to me.
packages/vite/src/node/config.ts
Outdated
@@ -1450,10 +1453,20 @@ export async function resolveConfig( | |||
.getSortedPluginHooks('configResolved') | |||
.map((hook) => hook.call(resolvedConfigContext, workerResolved)), | |||
) | |||
;(workerResolved.plugins as Plugin[]) = resolvedWorkerPlugins |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have a comment that explains why this need to mutate workerResolved.plugins
? Also do we also need to mutate workerResolved.environments
, or is that fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comments, and rearranged the code so it is easier to follow (hopefully). Also avoided recreating the worker resolved config after passing it to hooks (as they may store a reference) and fixed resolvedConfig for hooks, that should be after resolve the environment plugins ad08fee
Description
We recently discussed with @dominikg about environment plugins resolutions and he asked how to access the resolved plugins for a given environment. These are currently present at
environment.plugins
once the instance are inited. So during dev, there is the need to wait after server listen.We decided to pass a
PartialEnvironment
toapplyToEnvironment
to avoid committing to resolving the plugins after the Environment instance are created, and leave the door open to move the resolution to config time. This PR explores this change.The benefits are:
resolvedConfig.environments[name].plugins
.configResolved
can be used to read this values.config.plugins
always worked (they are fixed at config time, not after the server is created)The main difference with creating the plugins in the Environment instance is that if two Environments are created with the same config, they will share the environment plugins after this PR. We only allow a single server per ResolvedConfig, and we should do the same for Environments so this doesn't seems like an issue to me.