-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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: errors in worker handling #7236
Conversation
importAnalysis
plugin in worker bundle
importAnalysis
plugin in worker bundle
packages/vite/src/node/build.ts
Outdated
@@ -306,6 +306,7 @@ export function resolveBuildPlugins(config: ResolvedConfig): { | |||
post: Plugin[] | |||
} { | |||
const options = config.build | |||
const isWorker = config.isWorker |
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 think we should pass isWorker
as a param to resolveBuildPlugins
instead. Adding it to the config will make it visible to the final user
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'm not 100% sure if removing import analysis for workers is safe, but I have some small suggestions below.
I update desc
please cc |
Looks good to me. I've added it for discussion in the next team meeting due to the new |
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 LGTM too, but will have this discussed in the meeting as mentioned by patak.
Also some minor nitpicks below 😬
There are major changes to the script of the unit test, I don't know if this is good or not, please help to rereview 😘 |
We talked today about the PR with the team, it is ok to have |
Description
classic worker options analyse error and pop
importAnalysis
plugin in worker bundlepreLoad
is not support in worker(preload method use document). There is no CSS in the worker, so there is no need to load dependencies first and then modules so we can ignoreimportAnalyseBulld
jestPerTestSetup
add<testdir>/config.js
to config vite test serverformat: es
test for worker.@vite-ignore
error message add postionAdditional context
What is the purpose of this pull request?