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

add support for webpack4 #118

Merged
merged 2 commits into from
Feb 26, 2018
Merged

add support for webpack4 #118

merged 2 commits into from
Feb 26, 2018

Conversation

andriijas
Copy link
Contributor

@andriijas andriijas commented Feb 8, 2018

Please review carefully, Im not sure the PR is complete. I have no deep knowledge of webpack plugin internals.

I tested it in a small reference app and got a manifest generated.

Feedback appreciated.

Fixes #111

@codecov-io
Copy link

codecov-io commented Feb 8, 2018

Codecov Report

Merging #118 into master will decrease coverage by 5.74%.
The diff coverage is 72.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #118      +/-   ##
==========================================
- Coverage     100%   94.25%   -5.75%     
==========================================
  Files           2        2              
  Lines          78       87       +9     
==========================================
+ Hits           78       82       +4     
- Misses          0        5       +5
Impacted Files Coverage Δ
lib/plugin.js 94.18% <72.22%> (-5.82%) ⬇️

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 409b0fb...38181ac. Read the comment docs.

@mastilver
Copy link
Contributor

Thank you very much for that! 👍

Can you edit the .travis.yml so test are running for v4

@andriijas
Copy link
Contributor Author

I updated it but I think we will have to wait until webpack-contrib/extract-text-webpack-plugin#707 to improve webpack 4 testing strategies.

@mastilver
Copy link
Contributor

mastilver commented Feb 8, 2018

Cool tests are passing, I will merge that this week-end (remind me if I don't... ;) )

@weaverryan
Copy link
Contributor

@mastilver Ping also on making a stable 2.0 release :) :)

@ganapativs
Copy link

@mastilver When can we expect this to get merged? 😬

@irudoy
Copy link

irudoy commented Feb 14, 2018

Also, webpack-contrib/extract-text-webpack-plugin#707 has been merged 🎉

@andriijas
Copy link
Contributor Author

andriijas commented Feb 14, 2018

For me its totally OK to upgade all dependencies, cut a 2.0 release of the plugin only working with webpack 4 and clean up old plugin API but I guess most people want to have a transition period.

Just let me know if there is anything I can do to improve the PR.

Thanks

lib/plugin.js Outdated
@@ -61,7 +59,7 @@ ManifestPlugin.prototype.apply = function(compiler) {
path: path,
chunk: chunk,
name: name,
isInitial: chunk.isInitial ? chunk.isInitial() : chunk.initial,
isInitial: chunk.isInitial ? chunk.isInitial() : chunk.isOnlyInitial(),
Copy link

Choose a reason for hiding this comment

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

isInitial is only in v3.
So, I think l62 will throw error in v1/v2. and it should be like this.

isInitial: chunk. isOnlyInitial ? chunk. isOnlyInitial() : (chunk.isInitial ? chunk.isInitial() : chunk.initial),

@mastilver
Copy link
Contributor

@andriijas Thank you for the great work, let me know if you want ot be more involved into this project! :)

@ALL sorry for the delay, I didn't have much time. My boss just granted me permission to work on open source for the day, so I'm going to make the most out of it :)

@andriijas
Copy link
Contributor Author

@mastilver Thank you for letting me, I honestly dont have any deep knowledge of webpack I tried my best using the migration guide: https://medium.com/webpack/webpack-4-migration-guide-for-plugins-loaders-20a79b927202

Im still not sure how callbacks changed with the new plugin API

@mastilver
Copy link
Contributor

hum... weird, the build is now failing... maybe a dependency changed?

@mastilver
Copy link
Contributor

Found where it's coming from, a test :)

@andriijas
Copy link
Contributor Author

the test suite seems to run on webpack 3.x and maybe it should for a while. I read somewhere webpack 5 will be out in roughly 6 months, by then we can deprecate old plugin format.

Maybe we should use tapAsync though if we want that callback support ref: facebook/create-react-app#4077 (comment)

@mastilver
Copy link
Contributor

No, I'm talking about our unit tests and also I think we are missing: webpack-manifest-plugin-after-emit now on webpack 4

@andriijas
Copy link
Contributor Author

@mastilver check new PR

@mastilver
Copy link
Contributor

v2 is now available 🎉

Thank you @andriijas for your work, and all for your patience

@andriijas
Copy link
Contributor Author

no worries, just been learning while helping. thanks finishing up!

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

Successfully merging this pull request may close these issues.

7 participants