-
-
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
fix!: worker.plugins is a function #14685
Conversation
Co-authored-by: jamsinclair <jamsinclair@users.noreply.github.com>
Run & review this pull request in StackBlitz Codeflow. |
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
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.
General logic looks good to me 👍 Got a couple suggestions below to improve a bit.
...workerNormalPlugins, | ||
...workerPostPlugins, | ||
] | ||
workerConfig = await runConfigHook(workerConfig, workerUserPlugins, configEnv) | ||
const resolvedWorkerOptions: ResolvedWorkerOptions = { |
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.
Maybe we can move the workers resolve code as a function like resolveWorkerOptions
? So it fits the pattern like the other more complex options. Might need a new file beside config.ts
.
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.
I thought about that but it is using internal functions like filterPlugin
, and I wanted to avoid changing unrelated parts on this PR. Maybe we could do that refactoring in another PR?
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
It seems the errors in ecosystem-ci aren't related to this PR, the same fails are right now in main. |
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.
Looks mostly good 👍 I have small suggestions and a question.
@@ -75,7 +52,7 @@ async function serialBundleWorkerEntry( | |||
const bundle = await rollup({ |
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.
Maybe it's useful if there's a debug log here that shows the plugin names like we had here?
https://github.com/vitejs/vite/pull/14685/files#diff-11e17761d4ecfee8f8fde15c6d79b7bc0260176396a30dfd8e6f6bbaf5af4745R829
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 need to see how to do this only once because it will be too noisy if not having the logs everytime for projects that have a lot of workers. I'd say we merge it so it is part of the release and do this in a future PR.
Fixes #13367
Description
This is a breaking change, changing the signature of
worker.plugins
to be a function instead of an array of plugins.We move to a function to be able to get a fresh array of plugins every time we need to bundle a worker entry with rollup during build. This allows us to revert #11218 and have workers bundling in parallel.
The PR adds some of the tests from #14113 (has @jamsinclair as co-author).
mergeConfig
is also modified soworker.plugins
is merged as if it were an array.worker.plugins()
modifications in theconfig
hook toconfig.worker
will be ignored.What is the purpose of this pull request?