-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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: build time deps optimization, and ensure single crawl end call #12851
Conversation
Run & review this pull request in StackBlitz Codeflow. |
if (seenIds.size === 0) { | ||
callOnCrawlEnd() |
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.
registeredIds.length === 0
to seenIds.size === 0
is also needed after #12609
@@ -1,5 +1,5 @@ | |||
let base = `/${self.location.pathname.split('/')[1]}` | |||
if (base === `/worker-entries`) base = '' // relative base | |||
if (base.endsWith('.js') || base === `/worker-entries`) base = '' // for dev |
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.
Started getting a lot of warnings in dev
Error: Failed to load url /classic-shared-worker.js/classic.js (resolved id: /classic-shared-worker.js/classic.js). Does the file exist?
❯ loadAndTransform packages/vite/src/node/server/transformRequest.ts:242:22
❯ processTicksAndRejections node:internal/process/task_queues:95:5
I don't know if this is the correct fix, seems the PR is exposing this issue?
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.
Seems to happen on main
too. This fix works for me.
I tested this PR. It fixes #12836 👍 |
Sorry the PR got more complex, but deps optimization during build was broken and we need to fix them in tandem with the changes in this PR. I could separate this into two PRs, but I may need both changes to make every test pass. If you run This commit vitejs/vite@ |
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
Same errors in ecosystem-ci as in main, counts as a good run. |
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.
Code LGTM 👍
Let's merge and release to get some testing. @dominikg is thinking that it may be less complex to have ids registered on resolveId without a promise, then clean them up in a post-plugin with a transform hook. We can explore to see if this would work in another PR. I think I tried and it didn't work before but it is worth trying again. |
Description
Fix regression introduced by:
The awaited promise for registered ids that start a worker bundle process where there is an import for a dependency internally will not be resolved. So when
registerWorkersSource
is called, we need to stop waiting for it. This worked fine when we awaited one id at a time, but #12609 added aPromise.allSettled
that could contain one of these ids. This PRs adds a new Promise indirection so we can resolve these ids promises and let theallSettled
finish.The PR also implements a potential fix for #12836, I think we may not need #12848 after this PR, still needs more testing. And a clean-up of the onCrawlEnd scheme, adding a way to cancel the current crawling.
What is the purpose of this pull request?