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 in asset filename filtering via {test, include, exclude} #1149

Merged
merged 2 commits into from
Jan 4, 2018

Conversation

jeffposnick
Copy link
Contributor

R: @jeffposnick @addyosmani @gauntface
CC: @anilanar

Fixes #935

As mentioned in the comments/the original issue, this takes advantage of a pattern native to webpack and used in other plugins (like UglifyJS) for filtering the precache manifest of webpack assets based on an object with test, include, and exclude properties.

It defaults to using RegExps for exclude that will match .map files and manifest*.js(on) files. While webpack gives developers flexibility to name their sourcemaps and asset manifest files whatever they'd like, those seem like the most common naming schemes (and they can be overridden).

The drawback of this PR is that it makes filtering more complicated: prior to this PR we implemented filtering based on chunk name white/blacklists, and now this introduces filtering based on asset name white/blacklists. (The chunk filtering is run first, then asset filtering.) There seems to be developer demand for supporting both, but this is another thing to try to explain clearly in the docs update (google/WebFundamentals#5567).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 88.127% when pulling a1943b8 on webpack-initial-filtering into 3fc0178 on v3.

@@ -31,6 +31,9 @@
"json-stable-stringify": "^1.0.1",
"workbox-build": "^3.0.0-alpha.3"
},
"peerDependencies": {
"webpack": "^2.0.0 || ^3.0.0"

Choose a reason for hiding this comment

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

This seems unusual to me, is it stipulating that workbox only works with Webpack 2 and 3 (i.e. not 4)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am following UglifyJS's lead here in limited the acceptable peer dependency: https://github.com/webpack-contrib/uglifyjs-webpack-plugin/blob/025262284c86196d0a4c4fe71e186132579629ec/package.json#L75-L77

(My understanding is that webpack 4 changes the plugin architecture anyway, so I think we should track that separately: #1151)

@@ -177,6 +179,13 @@ function getManifestEntriesFromCompilation(compilation, config) {

const manifestEntries = [];
for (const [file, metadata] of Object.entries(filteredAssetMetadata)) {
// Filter based on test/include/exclude options set in the webpack config.

Choose a reason for hiding this comment

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

Can you just add a comment on the end of this sentence along the lines "This matches behaviour of uglifyJS". Copying and pasting links is O.T.T. to get the gist of the point your are getting across.

Choose a reason for hiding this comment

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

It would also be worth calling out that we can take advantage of webpacks built in filtering (which for me is a better reason for this approach over something uglify does).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 88.127% when pulling 32f8ac9 on webpack-initial-filtering into 3fc0178 on v3.

@jeffposnick jeffposnick merged commit dde16ba into v3 Jan 4, 2018
@jeffposnick jeffposnick deleted the webpack-initial-filtering branch January 4, 2018 15:50
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.

3 participants