-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
base: main
Are you sure you want to change the base?
Conversation
|
e1064a2 to
bc5f904
Compare
| await import('./custom2.js') | ||
| console.log(await import('./custom0.js')) | ||
| console.log(await import('./custom1.js')) | ||
| console.log(await import('./custom2.js')) |
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.
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.
This PR accomplishes this. |
775cf9b to
bc5f904
Compare
cd131ee to
1261ed3
Compare
| // To prevent dead code elimination. Will properly remove in generateBundle | ||
| // It's referenced twice so it doesn't get inlined by the minifier | ||
| ${preloadSaver}(${preloadMethod},${preloadMethod}); |
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.
While this can make the preloadMethod import to be kept, since this function call is treated as a sideeffect, rollup will treat this module as sideeffectful and would make the chunking less ideal.
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.
You're right. It's been a while, so correct me if I'm wrong, but doesn't this behavior exist with the current implementation as well?
To fix it, maybe we can create and inject the custom chunk import after all the chunks are rendered? Either way, I think this is still a big improvement compared to the build breakage that this PR resolves.
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.
doesn't this behavior exist with the current implementation as well?
The current implementation injects the preload method right next to the dynamic import and not at the root of the module. So it doesn't mark the module as side effectful, it only marks the function that contains the dynamic import.
To fix it, maybe we can create and inject the custom chunk import after all the chunks are rendered?
Yeah, this would solve this problem. But I didn't find a way to put the preload helper in a non-separate chunk. this.emitFile({ type: 'chunk' }) should work if a separate chunk is acceptable, though it's not ideal.
I think this is still a big improvement compared to the build breakage that this PR resolves.
I made a PR that at least fixes the breakage (#20117). But I’m open to switching to a different approach, as long as the trade-offs are worthwhile.
Description
fixes #19695
This PR fixes:
vitePreloadinjection so it doesn't get in the way of Rollup parsing named dynamic importsvitePreloadat all if there are no dependencies to loadI've broken this up into smaller PRs and I'm waiting for these to be reviewed and merged first:
importAnalysisBuild#19723Afterwards: