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: chunk split for dynamic import dep #6318

Closed
wants to merge 4 commits into from

Conversation

sanyuan0704
Copy link
Contributor

Description

For a dynamic import module,the application code and the third-party dep code will be seperated.In the meanwhile, each async-vendor will be divided in terms of dynamic import entry, instead of all being packaged into a single file, so huge async vendor can be avoided.

See details in #6289.

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@Niputi
Copy link
Contributor

Niputi commented Dec 30, 2021

will this fix #6250?

@sanyuan0704
Copy link
Contributor Author

sanyuan0704 commented Dec 30, 2021

will this fix #6250?

Yes,it is a general solution for the scene with dynamic import.

@Niputi Niputi linked an issue Dec 30, 2021 that may be closed by this pull request
7 tasks
@Niputi Niputi requested a review from antfu December 30, 2021 15:09
packages/vite/src/node/build.ts Outdated Show resolved Hide resolved
@Shinigami92
Copy link
Member

@QiroNT could you test this PR for us?

@QiroNT
Copy link

QiroNT commented Dec 30, 2021

could you test this PR for us?

I don't particularly like this implementation since it still kinds of combining modules imported from the same package, not ideal for stuffs like localization files.

Maybe should have an option to either combine modules by dynamic entry, or combine by package name. Could be configured by matching entries like how the optimizeDeps does.

BTW I think that localization files being split into two files is not beneficial, maybe also a option to disable this optimization (include packages in source chunk) for an entry? or just disable for very small files (also could be the default to the config above), but idk if it could fit inside vite's build cycle.

@benmccann
Copy link
Collaborator

Why do we need to do any special vendor chunking? createMoveToVendorChunkFn has been a long-standing pain point. We've gotten lots of bug reports about it such as sveltejs/kit#1632 and #3731 and we've even disabled it ourselves on several sites such as svelte.dev.

The feature was originally added in f6b58a0#diff-aa53520bfd53e6c24220c44494457cc66370fd2bee513c15f9be7eb537a363e7. I can only guess at why it was added as there was no explanation provided at the time. I guess the idea is that the vendor chunk changes less frequently and will be better cached? But it ends up loading lots of code that's unnecessary for a page, adds complication, creates more chunks, and causes issues if libraries are used from multiple places so I find it quite hard to believe it's an overall win. Personally, I'd really love to just remove it, or at least make it optional and off by default.

@QiroNT
Copy link

QiroNT commented Dec 30, 2021

True, either split the dependency then we get #2672, or don't split the dependency then we get #3731.

@sanyuan0704
Copy link
Contributor Author

could you test this PR for us?

I don't particularly like this implementation since it still kinds of combining modules imported from the same package, not ideal for stuffs like localization files.

Maybe should have an option to either combine modules by dynamic entry, or combine by package name. Could be configured by matching entries like how the optimizeDeps does.

BTW I think that localization files being split into two files is not beneficial, maybe also a option to disable this optimization (include packages in source chunk) for an entry? or just disable for very small files (also could be the default to the config above), but idk if it could fit inside vite's build cycle.

I understand.This solution separates application code and dependent code, improves the cache hit rate, and increases the number of chunks. Maybe you don’t like this way too much, but prefer to split the package size by package name.Such as:

{
  'markdown-it': ['markdown-it'],
  'echarts': ['echarts'],
  'react': ['react', 'react-dom']
}

Then the packages in the value will be packaged together.right?

@sanyuan0704
Copy link
Contributor Author

@QiroNT @benmccann However, currently vite's chunk split is implemented through rollup's manualChunks. If vite is not handled by default but a new chunk split configuration is created, it will conflict with build.rollupOptions.output.manualChunks.

@QiroNT
Copy link

QiroNT commented Dec 31, 2021

Maybe you don’t like this way too much, but prefer to split the package size by package name.

Actually it's the opposite, look closely at locales from date-fns, they should be packed per locale, which is how I defined in the dynamic import.

@QiroNT
Copy link

QiroNT commented Dec 31, 2021

True, either split the dependency then we get #2672, or don't split the dependency then we get #3731.

Thought for the past few hours, the whole vendor thing should be rewritten with this issue in mind. Probably doing an analyze to choose whether multiple entries from a dependency should be packaged together or not, I'll hopefully open a discussion about this proposal later, but god that's a lot more to consider than I've ever thought it would be.

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 module build problem
5 participants