refactor: plugin container for Vite 6 #16740
Conversation
|
|
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI on
✅ ladle, laravel, unocss, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-pages, vite-plugin-react-swc, vite-plugin-vue, vite-setup-catalogue |
|
I've yet to thoroughly review this, but wondering, could this be refactored in |
|
It's based on the new abstraction of the new env pluginConainter - it would require certain efforts to redo it in main - and then solving the conflicts in v6 as well. Do you think that would be worth the effort? |
There was a problem hiding this comment.
After reading the changes, I think making in main and then merging into v6 shouldn't be too hard. Plus we can get the improvements released quicker, and also easily test in ecosystem-ci of this change specifically.
In the future, I'd also like to fix this.load() behaviour not matching Rollup, and it would help reduce the merge conflicts later.
Personally I think it's worth it but it is still your call 😅
|
|
||
| addWatchFile(id: string) { | ||
| this._container.watchFiles.add(id) | ||
| // ;(this._addedImports || (this._addedImports = new Set())).add(id) |
| async load(id: string, options?: {}): Promise<LoadResult | null> { | ||
| const ssr = this.environment.name !== 'client' | ||
| options = options ? { ...options, ssr } : { ssr } | ||
| const ctx = new LoadPluginContext(this) |
There was a problem hiding this comment.
IIUC load() and transform() hooks still need to create its own context everytime? Seems like Rollup does this too with its replaceContext param, so we still need to track _addedImports and call updateModuleLoadAddedImports.
| const ctx = this._getPluginContext(plugin) | ||
| ctx._resolveSkips = skip | ||
| ctx._scan = scan | ||
|
|
There was a problem hiding this comment.
Re _resolveSkips, is this safe? Wouldn't it leak and share to other hooks too, and then this.resolve() would incorrectly skip the plugin?
There was a problem hiding this comment.
You are right, I fixed it in #17288 by having per-hook context. Rollup does this as well.
|
Ok, I will try to redo this on main, so we could also at least ensure it works without env api |
antfu
changed the title
|
Made the change/migration in |
This PR refactors the plugin containers file:
PluginContextper-plugin, instead of per-hook, align with rollup: https://github.com/rollup/rollup/blob/1b62c336b8e927e846cd2c04563ae0868a5d5832/src/utils/PluginDriver.ts#L105-L110ContextandTransformContextclasses to top-level instead of having them in the function that creates every time.EnvironmentPluginContainera class to have more explicit deps relationships and easier to work with