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): avoid __vitePreload breaking rollup dynamic import tree-shaking #17367

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yuzheng14
Copy link

Description

The __vitePreload function breaks Rollup's dynamic import tree-shaking.

Consider the following entry file example:

async function main() {
  const { foo, bar } = await import("./foo");
  console.log(foo, bar);
}

main();

And the foo.ts file:

export const foo = 1;
export const bar = 2;
export const baz = 3;
export const qux = 4;

Bundle this using Rollup will result in entry file index.js:

async function main() {
    const { foo, bar } = await import('./foo-i1IIQb7W.js');
    console.log(foo, bar);
}
main();

And file foo-i1IIQb7W.js:

const foo = 1;
const bar = 2;

export { bar, foo };

However, if bundled using Vite in non-lib mode, it generates entry file index-BY2O_S30.js inside index.thml:

/* some other code inserted by Vite */
async function main() {
  const { foo, bar } = await __vitePreload(() => import("./foo-B2TMQ0vo.js"), true ? [] : void 0);
  console.log(foo, bar);
}
main();

And the file foo-B2TMQ0vo.js:

const foo = 1;
const bar = 2;
const baz = 3;
const qux = 4;
export {
  bar,
  baz,
  foo,
  qux
};

The reason that tree-shaking of dynamic imports fails is that the __vitePreload function only wraps the import(/* module name */), but Rollup needs additional information to perform tree-shaking on the imported module.

To address this, I used AST to analyze what should be wrapped in the __vitePreload function.

After testing, I found two ways to consistently trigger Rollup's tree-shaking of dynamically imported modules:

  • import('./foo').then( ({ foo, bar }) => /* use foo and bar */)
  • const { foo, bar } = await import('./foo')

For the first case, I wrap the full import expression and its first then call expression into the __vitePreload function.

For the second case, I add a then call expression to indicate which exported content will be destructured and wrap it into the __vitePreload function.

After modification, Vite generates the entry JS file:

/* some other code inserted by Vite */
async function main() {
  const { foo, bar } = await __vitePreload(() => import("./foo-B4NibxfM.js").then(({ foo: foo2, bar: bar2 }) => ({ foo: foo2, bar: bar2 })), true ? [] : void 0);
  console.log(foo, bar);
}
main();

And the file foo-B4NibxfM.js:

const foo = 1;
const bar = 2;
export {
  bar,
  foo
};

Now it could tree-shake the dynamically imported module just as in Rollup!

I have used this modified Vite in production environment for my corporation, and it hasn't received any bug reports yet.

Copy link

stackblitz bot commented May 31, 2024

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

@yuzheng14
Copy link
Author

@bluwy Could you please take some time to review this pull request? I saw you that made changes to this file not long along.

@bluwy
Copy link
Member

bluwy commented Jun 4, 2024

We actually have another PR for this: #14221. I should've got that over the line and I don't remember why I left it there 😅 Sorry for the duplicated work.

Do you have thoughts on how the other PR work? (it uses regex instead of AST) I can try to push this fix in for the current beta. Also cc @sun0day

@yuzheng14
Copy link
Author

yuzheng14 commented Jun 4, 2024

We actually have another PR for this: #14221. I should've got that over the line and I don't remember why I left it there 😅 Sorry for the duplicated work.

Do you have thoughts on how the other PR work? (it uses regex instead of AST) I can try to push this fix in for the current beta. Also cc @sun0day

I've seen #14221 , it also matches the (await import(/* some module */)).foo, but sometimes Rollup will not trigger tree-shaking for this so that I didn't match this case.

I haven't made some benchmarks about this so that I don't know whether AST or regex is faster. But I compile my pr version in my company, there's no functional faults.

It's up to you to decide which branch should be merged.

@bluwy
Copy link
Member

bluwy commented Jun 6, 2024

Alright. I'll go ahead with #14221 for now and we can see how well it works. Otherwise we can reference this AST implementation in the future.

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.

None yet

2 participants