-
Notifications
You must be signed in to change notification settings - Fork 512
fix(index): stricter check for shouldExtract !== wasExtracted
#605
fix(index): stricter check for shouldExtract !== wasExtracted
#605
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add test for this?
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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? |
@kostasmanionis we have example of tests in |
shouldExtract !== wasExtracted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kostasmanionis Where/When does this currently fail?
@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 |
@kostasmanionis Yep, sry I missed it :). hmm... current tests are passing and the change is minimal, async CSS chunks are a known problem with |
2d799de
to
3730b83
Compare
@michael-ciniawsky tried to add a useful comment there, looks good? |
@kostasmanionis How difficult to implement the test? If your create minimum test repo we can help with this, thanks! |
@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 🤔 |
@kostasmanionis thanks, in near future i can investigate this - how we can implement test for this |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems it is difficult to test this 😞 Let's merge this, comment is enough. Maybe in future we found way to tests this. Thanks!
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? |
@kostasmanionis in the near future 👍 |
@d3viant0ne need third approved 👍 |
Tried this on a few projects that been having this issue and it works
Issues
Fixes #604