Changes for the v3 release

[jeffposnick]

A lot of lines of code have changed in this PR, but the changes can roughly be broken down into:

• Refining the language in the README.
• Adding support for webpack v4, including running the same integration test suite against webpack v3 and v4.
• Explicit support for node 6 and higher via @babel/present-env, dropping compatibility with node v4.
• Refactoring the codebase into smaller modules, and running unit tests against some of them.
• More robust input validation, and additional error messages.

The most visible functional change is webpack v4 support, with most of the other changes being of the under-the-hood variety.

@addyosmani, if you could focus on the changes to the README (with whatever level of detail you consider appropriate for the other files), that would be appreciated!

The alpha/betas have been used by several folks in the community, including @developit

Fixes #47

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

[developit]

You might find this nicer than having to duplicate the hooks:
https://github.com/GoogleChromeLabs/critters/blob/c53df3d6b1f1b63b6374d79b0fa565c48e60a85f/src/util.js#L1

[jeffposnick]

That's a good pattern in general.

We ran into issues with folks who listed this plugin prior to the HtmlWebpackPlugin, or just didn't list HtmlWebpackPlugin at all, and in v4 we can detect that at https://github.com/GoogleChromeLabs/preload-webpack-plugin/pull/72/files/b1f8ec65d655dfa064fad73765c6c552dc089a38#diff-1fdf421c05c1140f6d71444ea2b27638R135 and show a meaningful error message. I think it's worthwhile keeping the tapping logic explicit in this case since we're doing that detection.

[developit]

Not sure if any of the values have this issue, but a quote within a value seems like it would break here. Something like this would circumvent the issue.

[jeffposnick]

JSON.stringify() generally does a good job of making sure that you get back a properly-escaped string, regardless of inner quote characters. I've got a unit test to confirm that:

preload-webpack-plugin/test/unit/create-html-element-string.js

Lines 94 to 102 in b1f8ec6

	it(`should properly escape the attribute values as strings`, function(done) {
	const elementString = createHtmlElementString({
	elementName: 'test',
	attributes: {
	string: `Strings: '"\``
	}
	});
	expect(elementString).toEqual('<test string="Strings: \'\\"`">');

Does that address your concern, or am I missing a possible edge case?

[jsg2021]

Can this be left out? lib-size isn't as important for webpack plugins. I'm seeing issues with the runtime not getting hoisted correctly in node_modules.

[jsg2021]

Actually, I don't think it's a node_module structure issue. I think the latest babel beta changed the runtime api... this project probably needs to be recompiled and published... preferably without the runtime transform.

[jeffposnick]

I've bumped all the dependencies in this commit, including babel-related ones to 7.0.0-beta.56. Does that help at all?

That being said, I'm not sure why shipping a project like this that relied on @babel/runtime as a dependency would be an issue. Is there any official guidance from the babel team about that?

[jsg2021]

The runtime afaik is only meant to reduce babel helper duplication in libraries meant to be bundled. In my opinion, it's not terribly useful for code that is meant to run on node. If not included your code will run just the same. Since you are using the env preset for node6+ there isn't much being transformed.

Here is the docs on that transform: https://babeljs.io/docs/en/next/babel-plugin-transform-runtime.html

[jeffposnick]

Okay, fair enough. If it helps, I've removed the transform-runtime in the latest commit.

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[jeffposnick]

After discussing with @addyosmani, I'm just going to merge this into master and we'll move forward from there with future changes.

[addyosmani]

Thank you, Jeff!

…

On Wed, Aug 21, 2019, 1:56 PM Jeffrey Posnick ***@***.***> wrote: Merged #72 <#72> into master. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#72?email_source=notifications&email_token=AAA3C2M3JXZ6PL7WOYNSRDDQFWTZNA5CNFSM4FD4KNF2YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOTFZXPYA#event-2574481376>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAA3C2JE3MDIWMQDHLM7SX3QFWTZNANCNFSM4FD4KNFQ> .