Closed andriijas closed 6 years ago
@andriijas You should remove Node 4 from Travis also. Node.js 4 is no longer supported. Source Code was upgraded to a higher ecmascript version.
Yeah, good point! node@4 will be deprecated in April, so it's pretty safe to do it now
Merging #128 into master will decrease coverage by
2.74%
. The diff coverage is84.61%
.
@@ Coverage Diff @@
## master #128 +/- ##
==========================================
- Coverage 94.89% 92.15% -2.75%
==========================================
Files 2 2
Lines 98 102 +4
==========================================
+ Hits 93 94 +1
- Misses 5 8 +3
Impacted Files | Coverage Ξ | |
---|---|---|
lib/plugin.js | 92.07% <84.61%> (-2.77%) |
:arrow_down: |
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 052970e...1935847. Read the comment docs.
Call for help. π±π±π±
I went through π and π₯ to get the tests working properly on all versions of webpack and extract text plugin and current status is that tests are running/failing randomly.
Check https://travis-ci.org/danethurber/webpack-manifest-plugin/builds/347232114
TypeError: used is not a function
and sometimes
SyntaxError: Unexpected end of JSON input
There is something Im not getting right with the async resolving but I cant figure it out. Please advise.
Don't worry about SyntaxError: Unexpected end of JSON input
it's just a racing condition issue (which we will be able to get rid once we stop supporting webpack < 3)
TypeError: used is not a function
is a known issue https://github.com/mafintosh/mutexify/issues/7, just leave the current version for mutexify
. I'm getting rid of it anyway in #107 so you are good :)
@mastilver okay cool. then the bigger question is whatever compiler.hooks.webpackManifestPluginAfterEmit should use a sync or async API
I will squash the commits and force push when you have decided, dont want that ugly console.log in the history.
I think we want it sync for the future (So we won't need the lock system) but I could be wrong
I'm sorry, I just merged something and there is some merge conflicts now... :/
@mastilver rebased.
@mastilver anything i need to adjust? would be so thankful for a new release!
We need to figure out where the memory leak come from, I did some tweaking but I was unsuccessful... :/
@mastilver They are all failing when extract text is in Beta, might be that is the issue.
I tried this pull request with multiple instances of extract-text-webpack-plugin
. It didn't work. Looks like it simply chose the last instance of extract-text-webpack-plugin
and mapped that, while the first instance's output was discarded/overwritten.
extract-text-webpack-plugin is now in maintainance mode for webpack 3.
Maybe we should add https://github.com/webpack-contrib/mini-css-extract-plugin for webpack 4 tests.
it will require some work though
Oh, so extract-text-webpack-plugin
won't ever be released for webpack 4?
I think we should just ignore that test for now... release v2 then release v3 with just support for webpack@4 (the failing test is something that is happening very rarely)
Maybe check if we are running webpack@4, then return from the test
What do you think?
Sounds goodπ
@mastilver Looks like extract-text-webpack-plugin
support for webpack 4 support is being addressed on webpack-contrib/extract-text-webpack-plugin#685
Please take a look at https://github.com/webpack-contrib/mini-css-extract-plugin, it was build for webpack 4 as a replacement for extract-text-webpack-plugin
, please note that as its name says, it is intended for CSS extraction, no any text like extract-text-webpack-plugin
.
@rossta nope thats obsolete PR. check webpack-contrib/extract-text-webpack-plugin#749
@mastilver updated PR with some branching/if statements to detect webpack version in tests. Added test with MiniCSSExtractPlugin for webpack >=4
didnt have time for this but it had to be done π
@andriijas Sorry, I'm opening a new PR for this, we can take care of the refactoring later on, I just want to get it working with webpack@4 ;)
Do you mind reviewing #134
Thank you, so much :star:
The failing test is there: https://github.com/danethurber/webpack-manifest-plugin/blob/master/spec/plugin.integration.spec.js#L74