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 rollup dependency removal #442

Merged
merged 5 commits into from
Jul 5, 2018
Merged

Fix rollup dependency removal #442

merged 5 commits into from
Jul 5, 2018

Conversation

tivac
Copy link
Owner

@tivac tivac commented Jul 5, 2018

Fixes #441

What I originally thought was a tree-shaking bug turned out to be way more complicated.

So, here's what was going on.

  • I changed the transform hook to check if a file was known to the Processor instance and then remove the file being transformed, all the files that depended on that file, and all the files it depended on.
  • This change makes sense in the context of watching files for changes given how rollup doesn't tell us exactly what file changed so the tree needs to be aggressively pruned to avoid stale output.
  • But that assumption was real bad in the case where an already-populated Processor instances was passed to the rollup plugin (like when using modular-css-svelte).
  • Since that tree wasn't being re-processed the new aggressive removal logic would end up leaving holes in the tree due to the tree-walking order.

So, how to fix it? The answer for now is to support both flows!

If a file is already in the tree the code needs to check how many times it's run a build. If it's the first time do the light removal that only removes that file and the files that depend on it. If this is a watch-triggered multi-build then the aggressive removal step needs to be taken to also remove all the files the target file depends on.

phew

tivac added 5 commits July 1, 2018 23:46
No luck though, need to get a better repro.
Not sure why I changed it to the file and all its dependencies instead of just the default "file and its dependants" behavior, but that was the WRONG choice.
See comment on line 53 for the reasoning, it's goofy.
@tivac tivac self-assigned this Jul 5, 2018
@codecov
Copy link

codecov bot commented Jul 5, 2018

Codecov Report

Merging #442 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #442      +/-   ##
=========================================
+ Coverage   99.09%   99.1%   +<.01%     
=========================================
  Files          31      31              
  Lines         776     780       +4     
  Branches      119     120       +1     
=========================================
+ Hits          769     773       +4     
  Misses          7       7
Impacted Files Coverage Δ
packages/rollup/rollup.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ce23fa...6ad3858. Read the comment docs.

@tivac tivac merged commit fb6c61a into master Jul 5, 2018
@tivac tivac deleted the rollup-treeshaking branch July 5, 2018 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tree-shaken CSS causes build breaks
1 participant