Skip to content
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

Merged
merged 8 commits into from Apr 14, 2023

Conversation

patak-dev
Copy link
Member

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 a Promise.allSettled that could contain one of these ids. This PRs adds a new Promise indirection so we can resolve these ids promises and let the allSettled 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?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@stackblitz
Copy link

stackblitz bot commented Apr 13, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Comment on lines +736 to +737
if (seenIds.size === 0) {
callOnCrawlEnd()
Copy link
Member Author

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
Copy link
Member Author

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?

Copy link
Member

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.

@patak-dev patak-dev added the p4-important Violate documented behavior or significantly improves performance (priority) label Apr 13, 2023
@sun0day
Copy link
Member

sun0day commented Apr 14, 2023

I tested this PR. It fixes #12836 👍

@patak-dev
Copy link
Member Author

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 pnpm run build in playground/optimized-deps on main, you'll see that there are missing deps messages appearing and that the page will be reloaded (?). This message makes no sense during build because we should never reach it. During build, all non-optimized deps sources needs to be awaited first. So we always have the complete list of dependencies. I don't know for how long we had this issue (it may be that some of the tests added to optimize-deps lately started to show a case that was never handled)

This commit vitejs/vite@c7c9e6c (#12851) fixes the build issue.

@patak-dev
Copy link
Member Author

@sun0day would you retest for #12836 when you have some time?

@patak-dev
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Apr 14, 2023

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ❌ failure
iles ❌ failure
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt ❌ failure
previewjs ✅ success
qwik ✅ success
rakkas ✅ success
sveltekit ✅ success
vite-plugin-ssr ❌ failure
vite-plugin-react ✅ success
vite-plugin-react-pages ✅ success
vite-plugin-react-swc ✅ success
vite-plugin-svelte ✅ success
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ✅ success
windicss ✅ success

@patak-dev patak-dev changed the title fix: ensure single crawl end call in optimizer fix: build time deps optimization, and ensure single crawl end call Apr 14, 2023
@patak-dev patak-dev added the feat: deps optimizer Esbuild Dependencies Optimization label Apr 14, 2023
@patak-dev
Copy link
Member Author

Same errors in ecosystem-ci as in main, counts as a good run.

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM 👍

@patak-dev
Copy link
Member Author

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.

@patak-dev patak-dev merged commit fa30879 into main Apr 14, 2023
19 checks passed
@patak-dev patak-dev deleted the fix/ensure-single-crawl-end-call branch April 14, 2023 15:21
@sun0day
Copy link
Member

sun0day commented Apr 15, 2023

@sun0day would you retest for #12836 when you have some time?

Works fine! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: deps optimizer Esbuild Dependencies Optimization p4-important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants