-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
docs: missing changes guides #18491
docs: missing changes guides #18491
Conversation
const state = new Map<Environment, { count: number }>() | ||
return { | ||
name: 'count-transformed-modules', | ||
perEnvironmentBuildStartEnd: true, |
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.
Does perEnvironmentBuildStartEnd
still exist? The search result does not return any usage only the declarations: https://github.com/search?q=repo%3Avitejs%2Fvite+perEnvironmentBuildStartEnd&type=code
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.
Sorry, it is actually called perEnvironmentStartEndDuringDev
. We need a better name for this. Any recommendations?
About this option, the problem we have is that plugins my be expecting a single buildStart
call during dev, even if there are multiple environments, so I don't think we can start calling it multiple times in Vite 6. A lot of times it will work though, because it will end up reseting multiple times the state, but it could fail too. Moving to multiple buildStart
calls is something we haven't discuss much but I think it is a good idea to align dev and build
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.
Ah, that was it. I don't have any better naming 😅. But I guess we should rename perEnvironmentBuildStartEnd
to perEnvironmentStartEndDuringDev
and use the settings in the pluginContainer.
} | ||
``` | ||
|
||
To simplify this pattern, internally in Vite, we use a `useEnvironmentState` helper: |
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.
This is somewhat related to useEnvironmentState
helper. It's not directly related to this PR, but putting it here for now. I had to create a different plugin instance per-environment when using native plugins with Vite + Rolldown.
The implementation was like:
https://github.com/rolldown/vite/blob/f633bd2ceeaf3d3f28d6c9940977343b12f89ac6/packages/vite/src/node/plugin.ts#L339-L380
https://github.com/rolldown/vite/blob/f633bd2ceeaf3d3f28d6c9940977343b12f89ac6/packages/vite/src/node/plugins/index.ts#L85-L97
I'm not sure if this would be needed commonly, but maybe something similar to this is needed when using some of the rollup plugins in npm.
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.
We discussed with Anthony about a similar helper when we were trying to see how to refactor the reporter plugin: https://github.com/vitejs/vite/pull/17292/files
The api.getBuiltinPlugin
trick is interesting. I also think we're going to need a better story to make rollup plugins per environment. When we had the functional API for plugins it was very easy, just wrap it as () => rollupPlugin()
and you're done. Same with the object based { environmentPlugins: () => rollupPlugin() }
.
We ended up ditching the first one because of back compat, and the idea in general (the second was ok regarding compatibility) because we didn't want to push for the concept of two types of plugins:
- Shared plugins (environment aware)
- Per-environment plugin
The user now needs to know that a per-env plugin can't be in the main pipeline. And that a shared plugin (that actually shares data across env) needs to be always used as a regular plugin.
But maybe it is unavoidable, because with the current API, we are just ignoring that all these rollup plugins exists. When we tried to make alias per plugin, we didn't have a good way to do it internally either.
I wonder if we should extend to applyToEnvironment(environment) => boolean | plugin
, and so we can have something similar to what you did in rolldown-vite with a helper
{
name: 'per-environment-alias',
applyToEnvironment(environment) => aliasPlugin(environment.config.alias)
}
If an object is returned instead of a boolean, it replaces the current plugin with it for this environment.
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 user now needs to know that a per-env plugin can't be in the main pipeline. And that a shared plugin (that actually shares data across env) needs to be always used as a regular plugin.
Yeah, the user might sometime need to understand it. But I think that doesn't happen much and the decision of making shared plugins by default was good.
I wonder if we should extend to
applyToEnvironment(environment) => boolean | plugin
, and so we can have something similar to what you did in rolldown-vite with a helper
That would work too. I went with
makeEnvironmentAwarePlugin('per-environment-alias', env => aliasPlugin(env))
because it felt more intuitive from a plugin user standpoint for me (the metal model is that this function makes the plugin env-aware).
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.
we should still discuss about exposing
usePerEnvironmentState
Personally I'm still not sure what's the best way for this, but if you think there's a specific pattern that feels safe to recommend it to use, maybe we could export it. Otherwise we could leave for the ecosystem to explore and see where it converges on
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 comments from me are orthogonal to this PR and the PR itself looks good to me.
Description
We can merge this PR, but we should still discuss about exposing
usePerEnvironmentState
. We could:@vitejs/plugin-utils
lib