[Feature] Support html-webpack-plugin 4.x #16

sibiraj-s · 2018-09-20T17:32:46Z

Summary

Support html-webpack-plugin-4.x

Requirements

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.
I've written tests to cover the new code and functionality included in this PR.
I've read, agree to, and signed the Contributor License Agreement (CLA).

Closes #20

AnujRNair

Hi @sibiraj-s, thanks for the PR!
Some comments below :)

AnujRNair · 2018-09-20T17:37:57Z

package.json

@@ -31,7 +31,7 @@
  },
  "peerDependencies": {
    "webpack": "^2 || ^3 || ^4",
-    "html-webpack-plugin": "^2 || ^3"
+    "html-webpack-plugin": "^2 || ^3 || ^4.0.0-alpah.2"


Small sp mistake here: alpha

We'll probably wait for the major version to drop before merging this just incase the API changes, so let's also update this simply to ^4

😅 Will Update.

plugin.js

AnujRNair · 2018-09-20T18:15:31Z

Eventually we will remove them, but that would require a major bump - the more backward compatibility we can keep, the less we break this plugin for anyone else who might be using it at the moment

AnujRNair · 2018-09-20T18:22:06Z

I'll mark this as Ready to Merge, but won't actually merge until HTMLWebpackPlugin comes out of alpha and beta, just incase there are unforseen API updates

Thanks for the contribution!

codecov · 2018-09-21T13:49:44Z

Codecov Report

Merging #16 into master will decrease coverage by 1.16%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master      #16      +/-   ##
==========================================
- Coverage   98.79%   97.63%   -1.17%     
==========================================
  Files           2        2              
  Lines         166      169       +3     
  Branches        9       10       +1     
==========================================
+ Hits          164      165       +1     
- Misses          2        4       +2

Impacted Files	Coverage Δ
plugin.js	`93.84% <50%> (-2.93%)`	⬇️

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 b0adaea...3232f4d. Read the comment docs.

sibiraj-s · 2018-09-28T13:22:01Z

Updated html-webpack-plugin to beta.

Saravanan90 · 2018-10-16T10:35:07Z

@AnujRNair FYR jantimon/html-webpack-plugin#953 (comment)

Saravanan90 · 2018-10-24T13:49:57Z

@AnujRNair Any ETA on merging this PR

AnujRNair · 2018-10-26T00:11:54Z

@sibiraj-s do you mind making the dependency ^ 4? Then I'll merge and release this so it's ready for when html-webpack-plugin@4 is released - it will also mean that devs can start using your changes now

sibiraj-s · 2018-10-26T03:02:45Z

done 👍

phil-lgr · 2018-12-06T02:34:42Z

Hey I just tried the version on this branch, works super for me!

one thing:

processCsp(htmlPluginData, compileCb) {
        const $ = cheerio.load(htmlPluginData.html, {
            decodeEntities: false
        });

        let metaTag = $('meta[http-equiv="Content-Security-Policy"]');

        // if not enabled, remove the empty tag
        if (!this.isEnabled(htmlPluginData)) {
            // HERE <-----------------------  better to just bail out and call callback
            return compileCb(null, htmlPluginData);
        }

otherwise

htmlPluginData.html = $.html();

will insert

<html><head></head><body></body></html> to the template!

phil-lgr · 2018-12-06T02:36:08Z

basically if disableCspPlugin: true I would not touch the template at all

AnujRNair · 2018-12-07T21:49:16Z

Hi @phil-lgr!

We added this line as some templates had empty CSP meta tags which we wanted to remove if the dev marked the plugin as disabled - we should expand this to check whether anything was actually removed or not - if so, return the new HTML, and if not, bail the plugin

Would you be willing to make a PR for this change?

phil-lgr · 2018-12-07T22:11:29Z

Hello!

I think it makes things more complicated:

We added this line as some templates had empty CSP meta tags which we wanted to remove if the dev marked the plugin as disabled

IMO if the plugin is disabled it should not touch the template at all. Removing an empty CSP policy when disableCspPlugin: true is confusing, disable should means don't touch my template no nothing.

People use html-plugin for other scenario than producing an index.html

I use it to create my manifest.json:

so I just want to stress that I need disableCspPlugin: true to not touch the template at all.

if you are ok with the above, I'll create a PR! 👍

phil-lgr · 2018-12-07T22:30:06Z

we can create another option, maybe cleanEmptyCsp: true | false if you want?

AnujRNair · 2018-12-19T21:24:07Z

I've merged html webpack plugin 4 support in this PR: #23 to avoid going back and forth over merge conflicts :) Thanks for adding this in for us!

phil-lgr · 2018-12-19T21:35:18Z

Hi, I'd appreciate if you could reply to: #16 (comment)

Again, disableCspPlugin: true should not do anything to the template

we need to remove htmlPluginData.html = $.html(); when disabling!

AnujRNair · 2018-12-19T21:39:26Z

Hi @phil-lgr - sorry for the delayed response!

I agree - this is an unfortunate side effect of simply including the plugin in your webpack config at the moment, and should change. I'll shortly be looking into how we can disable the plugin in a safe manner whilst still keeping backwards compatibility with the existing setup.

Otherwise I will be shortly adding more features into the plugin which will require a major version bump, and I'll change the behaviour of the setting then

Thanks!

phil-lgr · 2018-12-19T21:44:08Z

no worries, still appreciate the work done here

ok, I'll keep an eye on that update then, thanks

AnujRNair · 2018-12-20T01:07:23Z

@phil-lgr are you able to provide me with a minimal sample repo reproducing the issue you're seeing? The webpack config and the HtmlWebpackPlugin template file you're using to create your manifest would be extremely beneficial
Thanks!

phil-lgr · 2018-12-20T01:44:12Z

I will prepare it! On Wed, Dec 19, 2018 at 8:07 PM Anuj Nair <notifications@github.com<mailto:notifications@github.com>> wrote: @phil-lgr<https://github.com/phil-lgr> are you able to provide me with a minimal sample repo reproducing the issue you're seeing? The webpack config and the HtmlWebpackPlugin template file you're using to create your manifest would be extremely beneficial Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#16 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AJMrQ2_SCXGfOplff3PALj_aGWcPTxV3ks5u6uLNgaJpZM4WymEy>.

…

-- Phil Sent from Mobile

sibiraj-s · 2018-12-20T05:26:41Z

I suggest to open a new issue and track it separately, @phil-lgr If you think it is a bug or something else.

The discussion is out of topic to this PR

cc @AnujRNair

sibiraj-s changed the title ~~feat: suppot html-webpack-plugin 4.x~~ [Feature] Suppot html-webpack-plugin 4.x Sep 20, 2018

AnujRNair suggested changes Sep 20, 2018

View reviewed changes

AnujRNair added the Status: Ready to Merge label Sep 20, 2018

sibiraj-s changed the title ~~[Feature] Suppot html-webpack-plugin 4.x~~ [Feature] Support html-webpack-plugin 4.x Sep 20, 2018

sibiraj-s added 2 commits September 21, 2018 19:19

feat: support html-webpack-plugin 4.x

2c9180f

fix typo in peerDependncies

25845d3

support html-webpack-plugin@beta

941d5fc

AnujRNair mentioned this pull request Oct 9, 2018

Cannot read property 'tapAsync' of undefined when using with HTML Webpack Plugin 4.0.0-alpha.2 #20

Closed

9 tasks

AnujRNair added Status: In Review and removed Status: Ready to Merge labels Oct 26, 2018

update html-webpack-plugin stable version

e778be7

sibiraj-s added 2 commits November 1, 2018 17:33

Merge branch 'master' into html-webpack-plugin-4

657b3df

Merge branch 'master' into html-webpack-plugin-4

3232f4d

AnujRNair mentioned this pull request Dec 19, 2018

Adding HtmlWebpackPlugin@4 support #23

Merged

4 tasks

AnujRNair closed this Dec 19, 2018

AnujRNair removed the Status: In Review label Dec 19, 2018

AnujRNair mentioned this pull request Jan 2, 2019

If plugin is disabled, we should not modify the html at all #28

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Support html-webpack-plugin 4.x #16

[Feature] Support html-webpack-plugin 4.x #16

sibiraj-s commented Sep 20, 2018 •

edited

AnujRNair left a comment

AnujRNair Sep 20, 2018

sibiraj-s Sep 20, 2018

AnujRNair commented Sep 20, 2018

AnujRNair commented Sep 20, 2018

codecov bot commented Sep 21, 2018 •

edited

sibiraj-s commented Sep 28, 2018 •

edited

Saravanan90 commented Oct 16, 2018

Saravanan90 commented Oct 24, 2018

AnujRNair commented Oct 26, 2018

sibiraj-s commented Oct 26, 2018

phil-lgr commented Dec 6, 2018 •

edited

phil-lgr commented Dec 6, 2018

AnujRNair commented Dec 7, 2018

phil-lgr commented Dec 7, 2018

phil-lgr commented Dec 7, 2018

AnujRNair commented Dec 19, 2018

phil-lgr commented Dec 19, 2018

AnujRNair commented Dec 19, 2018

phil-lgr commented Dec 19, 2018

AnujRNair commented Dec 20, 2018

phil-lgr commented Dec 20, 2018 via email

sibiraj-s commented Dec 20, 2018

[Feature] Support html-webpack-plugin 4.x #16

[Feature] Support html-webpack-plugin 4.x #16

Conversation

sibiraj-s commented Sep 20, 2018 • edited

Summary

Requirements

AnujRNair left a comment

Choose a reason for hiding this comment

AnujRNair Sep 20, 2018

Choose a reason for hiding this comment

sibiraj-s Sep 20, 2018

Choose a reason for hiding this comment

AnujRNair commented Sep 20, 2018

AnujRNair commented Sep 20, 2018

codecov bot commented Sep 21, 2018 • edited

Codecov Report

sibiraj-s commented Sep 28, 2018 • edited

Saravanan90 commented Oct 16, 2018

Saravanan90 commented Oct 24, 2018

AnujRNair commented Oct 26, 2018

sibiraj-s commented Oct 26, 2018

phil-lgr commented Dec 6, 2018 • edited

phil-lgr commented Dec 6, 2018

AnujRNair commented Dec 7, 2018

phil-lgr commented Dec 7, 2018

phil-lgr commented Dec 7, 2018

AnujRNair commented Dec 19, 2018

phil-lgr commented Dec 19, 2018

AnujRNair commented Dec 19, 2018

phil-lgr commented Dec 19, 2018

AnujRNair commented Dec 20, 2018

phil-lgr commented Dec 20, 2018 via email

sibiraj-s commented Dec 20, 2018

sibiraj-s commented Sep 20, 2018 •

edited

codecov bot commented Sep 21, 2018 •

edited

sibiraj-s commented Sep 28, 2018 •

edited

phil-lgr commented Dec 6, 2018 •

edited