Skip to content

fix: apply __vitePreload correctly and when necessary #19722

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

privatenumber
Copy link
Contributor

@privatenumber privatenumber commented Mar 26, 2025

Description

fixes #19695

This PR fixes:

  • vitePreload injection so it doesn't get in the way of Rollup parsing named dynamic imports
  • Doesn't apply vitePreload at all if there are no dependencies to load
  • Deduplicates the chunk ID -> path mapping across chunks

I've broken this up into smaller PRs and I'm waiting for these to be reviewed and merged first:

Afterwards:

@privatenumber
Copy link
Contributor Author

privatenumber commented Mar 26, 2025

  • There's too many changes in here so I'm going to split them up into smaller PRs

await import('./custom2.js')
console.log(await import('./custom0.js'))
console.log(await import('./custom1.js'))
console.log(await import('./custom2.js'))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These imports actually have no side-effects so Rollup should have been tree-shaking them. However, the way the preload method was previously injected prevented them from getting tree-shaken.

Now that it's being applied correctly, Rollup accurately detects that their export values weren't being used, which is why I had to add console.logs here.

@privatenumber
Copy link
Contributor Author

@bluwy #16184 (review)

Thanks! This looks great to me. Something I'd also like to followup later is to remove the mapDeps altogether if no preloading is needed for the file. Currently it's added unconditionally. And the changes could get a bit big.

This PR accomplishes this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic import().then(m => m.foo) works in dev but fails in prod
1 participant