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

refactor: rewrite vendor chunk to handle redirect exports #6347

Closed
wants to merge 8 commits into from

Conversation

QiroNT
Copy link

@QiroNT QiroNT commented Jan 1, 2022

Description

fix #3731

I decided to not open a discussion, but this implementation should be more tested. I have no idea if it actually solved that kind of issue, but tested it solved that particular issue.

Before:
200KB vendor

After:
131KB vendor

And yes, performance is not great on large size projects, but I only have a medium size project, and here is the result:
1609 modules, 798.96ms collect redirects + 51.31ms trace imports

Additional context

The real problem is the current implementation of vendor doesn't handle redirect exports, dependencies like framer-motion provided a combined export kinds of like this:

// index.js
export { foo } from "./foo.js"
export { bar } from "./bar.js"

and the import looks like:

// main.js
import { foo } from "./index.js"

In the current implementation of vendor, the file index.js is considered to be a vendor file, so every import to index.js is put inside vendor chunk.

But rollup handles this situation just fine, when vendor chunk is disabled, rollup splits dynamic import to bar seperately.

This implementation scans all redirect exports. Instead of marking index.js as a vendor file, this pr detects foo import from main.js and only marks foo.js as a vendor file, fixes the issue.

#6318 probably should be based on this implementation.

Side note: this implementation still allows setting manualChunks to undefined to disable vendor chunk.


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.

@QiroNT
Copy link
Author

QiroNT commented Jan 1, 2022

@benmccann could you help me test this on that SvelteKit issue?

Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

Code looks okay
Is it possible to add a test?

@Shinigami92 Shinigami92 added p2-nice-to-have Not breaking anything but nice to have (priority) needs test labels Jan 1, 2022

/**
* Build only. Analyze which modules are used by entries and
* generate manualChunks config to split those files to vendor.
Copy link
Collaborator

Choose a reason for hiding this comment

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

this could use an explanation as to what the algorithm is and why this is being used instead of Rollup's default behavior

Copy link
Author

Choose a reason for hiding this comment

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

added a simple explanation, Is it okay?

@benmccann
Copy link
Collaborator

I didn't test it, but to be very blunt, my first reaction is that this is insanity. Rollup has built-in code splitting, so why build some very complicated thing to replace it? At the very least there needs to be some explanation of what this is doing and what its benefits are. My initial reaction is that I would be much happier to just rip it all out and not deal with the added complexity and issues caused by this feature, but maybe I would feel differently given some explanation

@Niputi Niputi removed the needs test label Jan 2, 2022
@QiroNT
Copy link
Author

QiroNT commented Jan 2, 2022

added tests, but the tests are very barebone. I have no idea what edge cases to cover since I just ignored exports in traceImport in the last commit and it results in 305 -> 203 vendor files decrease with no change in vendor chunk. But it seems to be working for all projects I personally tested with no node_modules in source chunk and a decrease (or none for some) in vendor chunk.

@QiroNT
Copy link
Author

QiroNT commented Jan 3, 2022

My initial reaction is that I would be much happier to just rip it all out and not deal with the added complexity and issues caused by this feature, but maybe I would feel differently given some explanation.

Started a discussion about this, we should talk about it with more users to decide whether we implement it or just get rid of vendor chunk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code splitting ineffective due to vendor chunk
4 participants