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

Conversation

Projects
None yet
1 participant
@tivac
Copy link
Owner

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 some commits Jul 2, 2018

test: try to replicate #441
No luck though, need to get a better repro.
fix: Only remove the file and its dependents
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.
fix: handle both removal cases
See comment on line 53 for the reasoning, it's goofy.

@tivac tivac self-assigned this Jul 5, 2018

@codecov

This comment has been minimized.

Copy link

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

4 checks passed

codecov/patch 100% of diff hit (target 99.09%)
Details
codecov/project 99.1% (+<.01%) compared to 6ce23fa
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@tivac tivac deleted the rollup-treeshaking branch Jul 5, 2018

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.