fix(index): stricter check for shouldExtract !== wasExtracted

[kostasmanionis]

Tried this on a few projects that been having this issue and it works

Issues

---

Fixes #604

[jsf-clabot]

All committers have signed the CLA.

[alexander-akait]

Can we add test for this?

[codecov]

Codecov Report

Merging #605 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #605   +/-   ##
=======================================
  Coverage   87.41%   87.41%           
=======================================
  Files           7        7           
  Lines         302      302           
  Branches       68       68           
=======================================
  Hits          264      264           
  Misses         36       36           
  Partials        2        2

Impacted Files	Coverage Δ
src/index.js	89.7% <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 6a660f3...3730b83. Read the comment docs.

[kostasmanionis]

I would love to add some tests, though I'm not really sure how to approach it 🤔 I would need to setup a webpack compiler, then trigger a rebuild and somehow check if the ids are correct... Or is there a simpler way?

[alexander-akait]

@kostasmanionis we have example of tests in https://github.com/webpack-contrib/extract-text-webpack-plugin/tree/master/test, without tests this PR most likely will not be accepted.

[michael-ciniawsky]

@kostasmanionis Where/When does this currently fail?

[kostasmanionis]

@michael-ciniawsky there's some info and a reproducible use case in #604

I'm not yet sure how to test the rebuild functionality 🤔 If anyone has any suggestions, let me know

[michael-ciniawsky]

@kostasmanionis Yep, sry I missed it :). hmm... current tests are passing and the change is minimal, async CSS chunks are a known problem with ExtractTextPlugin, I also don't know how to test for rebuilds tbh. Maybe add a comment above the check in case this f** up some other usecases and needs to be reverted :)

[kostasmanionis]

@michael-ciniawsky tried to add a useful comment there, looks good?

[alexander-akait]

@kostasmanionis How difficult to implement the test? If your create minimum test repo we can help with this, thanks!

[kostasmanionis]

@evilebottnawi there's a reproducible repo https://github.com/kostasmanionis/extract-text-plugin-rebuild-bug

I'm thinking I would need to trigger a webpack rebuild to test this specific use case, since the problem does not occur on the initial build. Then I would need to check if all of the module ids are correct 🤔

[alexander-akait]

@kostasmanionis thanks, in near future i can investigate this - how we can implement test for this

[alexander-akait]

@kostasmanionis good work, your solution fix issue, but without tests we can change behavior (our just refactor code) in future and get regression 😞 Need search how we can implement tests.

[alexander-akait]

Seems it is difficult to test this 😞 Let's merge this, comment is enough. Maybe in future we found way to tests this. Thanks!

[kostasmanionis]

Thanks for finding time to look at it!

I have a few ideas on how to test it 🤔 Gonna try playing with a bit when I have some time, seems like a good opportunity to get a little bit more familiar with webpack internals.

Any idea when this fix could get published?

[alexander-akait]

@kostasmanionis in the near future 👍

[alexander-akait]

@d3viant0ne need third approved 👍