fix: build time deps optimization, and ensure single crawl end call#12851
fix: build time deps optimization, and ensure single crawl end call#12851
Conversation
|
|
| if (seenIds.size === 0) { | ||
| callOnCrawlEnd() |
There was a problem hiding this comment.
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.
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:5I don't know if this is the correct fix, seems the PR is exposing this issue?
There was a problem hiding this comment.
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. |
|
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
registerWorkersSourceis called, we need to stop waiting for it. This worked fine when we awaited one id at a time, but #12609 added aPromise.allSettledthat could contain one of these ids. This PRs adds a new Promise indirection so we can resolve these ids promises and let theallSettledfinish.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?