Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

refactor: cache assets similar to how the normal compilation does it #325

Closed
wants to merge 1 commit into from
Closed

refactor: cache assets similar to how the normal compilation does it #325

wants to merge 1 commit into from

Conversation

yuriyostapenko
Copy link

Previously extract-text-webpack-plugin emitted css assets every time anything else changed.
Injected assets were also not included in compilation hash calculation and caused some rebuilds to be hidden.

There are no tests included in this PR, since this is watch-related and tests will go in a separate PR in the main repo that has great facilities for that.
This continues fixes started with webpack/webpack#3737 and webpack/webpack#3744 and should hopefully address a lot of watch related issues with extract-text-webpack-plugin.

@codecov-io
Copy link

codecov-io commented Jan 6, 2017

Codecov Report

Merging #325 into master will decrease coverage by 0.03%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #325      +/-   ##
=========================================
- Coverage   90.14%   90.1%   -0.04%     
=========================================
  Files           4       4              
  Lines         355     364       +9     
  Branches       75      78       +3     
=========================================
+ Hits          320     328       +8     
- Misses         35      36       +1
Impacted Files Coverage Δ
index.js 90.98% <88.88%> (-0.09%) ⬇️

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 e81b883...e5cfd05. Read the comment docs.

@bebraw
Copy link
Contributor

bebraw commented Jan 28, 2017

@d3viant0ne Any thoughts on this? Post final merge?

@bebraw bebraw added this to the 2.1 milestone Jan 28, 2017
@joshwiens joshwiens force-pushed the master branch 2 times, most recently from 664e78c to 76a171d Compare January 28, 2017 22:40
@michael-ciniawsky michael-ciniawsky changed the title Cache assets similar to how the normal compilation does it refactor: cache assets similar to how the normal compilation does it Apr 23, 2017
@michael-ciniawsky
Copy link
Member

@uncleyo Please close and reopen the PR to trigger the CLA Bot again, sign the CLA and rebase against current master 😛

Copy link
Member

@joshwiens joshwiens left a comment

Choose a reason for hiding this comment

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

As a general rule of thumb, if i'm adding a conditional check to trigger some sort of feature that is a place where I need to add a test.

Watch related or not, this should be testable and imo should not merge without one.

Previously extract-text-webpack-plugin emitted css assets every time anything else changed.
Injected assets were also not included in compilation hash calculation and caused some rebuilds to be hidden.
@jsf-clabot
Copy link

jsf-clabot commented Jun 8, 2017

CLA assistant check
All committers have signed the CLA.

@yuriyostapenko
Copy link
Author

I've rebased this PR just to refresh it to latest master. I Don't think it should be merged.

For anyone interested, I've been using this plugin https://gist.github.com/uncleyo/106c3cb407bf5dd38ebd6be0710afd4a as a workaround for a while now. If anyone else is missing this feature, feel free to use it.

I don't like the code in this PR here myself, honestly, as I believe this is something webpack should handle automatically., either by exposing something like compilation.addAsset() or with plugin, something like the one in my gist.

I'll not close the PR just yet, hoping to hear some comments

@michael-ciniawsky
Copy link
Member

@uncleyo Thx :) appeciated, well I'm not 💯 suited to give a meaningful answer about this I will try to bring @sokra in :D

@michael-ciniawsky
Copy link
Member

@uncleyo Would you be interested in make the gist into a plugin cache-webpack-plugin or the like :) ? It's caching assets 'only' atm right ?

@michael-ciniawsky
Copy link
Member

We are getting close to a new major release and therefore I will close this for now, feel free to open a new PR with these changes, when #540 landed on master, but as you commented this maybe better solved at core level in general 😛. Thx

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants