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

rollup plugin does incorrect CSS chunking #559

Closed
tivac opened this issue Jan 30, 2019 · 1 comment

Comments

Projects
None yet
1 participant
@tivac
Copy link
Owner

commented Jan 30, 2019

Expected Behavior

When creating files that contain a dep graph like this:

a.js -> a.css -> c.css
b.js -> b.css -> c.css

the rollup plugin should create a.css, b.css, and c.css in the output since c.css is required by both a.css and b.css

Current Behavior

a.css contains both a.css and c.css, and b.css contains just b.css.

Possible Solution

// How does Set not have .filter yet ಠ_ಠ
const included = [ ...css ].filter((file) => !queued.has(file));

This code is intended to prevent CSS from being output multiple times, which is a good idea. Unfortunately it also means that any particular CSS file will only be output once, in the first JS-triggered chunk that depends on it.

Instead the plugin should look at the JS chunk graph & the CSS chunk graph & figure out the minimal set of CSS chunks that satisfies all the dependencies.

Steps to Reproduce (for bugs)

https://gist.github.com/tivac/43b94e87927d8838305f2ae88fb0d789

Context

This issue means that it isn't safe to use import() with code that has CSS dependencies

Your Environment

Executable Version
modular-css v22.0.0
npm --version 6.5.0
node --version 10.15.0

@tivac tivac self-assigned this Jan 30, 2019

@tivac

This comment has been minimized.

Copy link
Owner Author

commented Feb 4, 2019

Almost done fixing this in the corrected-chunking branch.

@tivac tivac referenced this issue Feb 5, 2019

Merged

fix: finally wrote correct chunking logic #562

5 of 9 tasks complete

@tivac tivac closed this in #562 Feb 5, 2019

tivac added a commit that referenced this issue Feb 5, 2019

feat: processor.normalize() & corrected chunking logic (#562)
* fix: functional CSS chunking algorithm

Walks JS entries + CSS dep graph to determine ideal chunking locations and merges nodes together until it gets there. Probably a bit slower than before but *way* more accurate and safer to use.

* fix: change asset naming to not depend on entries

There were just too many issues around trying to parse out the source filename, so for now we use the CSS files instead.

* feat: processor.normalize()

Because sometimes you need external access to cleaned-up paths

Fixes #559 
Fixes #560
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.