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.

Sign up for GitHub

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

Jump to bottom

Workbox service worker #4169

Merged
merged 14 commits into from
Sep 27, 2018
Merged

Workbox service worker #4169

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
b792806
Add workbox service worker functionality
davejm Mar 17, 2018
4059079
Remove debug
davejm Mar 17, 2018
db75638
Set workboxConfig for when there isn't a cra config file
davejm Mar 17, 2018
7f17257
Remove workbox configuration options as c-r-a isn't planning on suppo…
davejm Mar 20, 2018
b25b6e0
Remove c-r-a config path from paths
davejm Mar 20, 2018
ee3c1ae
Add workbox service worker functionality
davejm Mar 17, 2018
9a2024e
Remove c-r-a config path from paths
davejm Mar 20, 2018
1a6f48d
Inline the webpack workbox config
davejm Mar 20, 2018
f3223c3
Merge branch 'master' into workbox-service-worker-config
Timer Sep 27, 2018
9a87859
Use settings reccommended by @jeffposnick
Timer Sep 27, 2018
3dcec0e
Fallback to public url index.html, not root
Timer Sep 27, 2018
88f603a
Add one comment
Timer Sep 27, 2018
285e663
Update comment
Timer Sep 27, 2018
7f6e7ef
Correct regex
Timer Sep 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 9 additions & 31 deletions packages/react-scripts/config/webpack.config.prod.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const UglifyJsPlugin = require('uglifyjs-webpack-plugin');
const ExtractTextPlugin = require('extract-text-webpack-plugin');
const ManifestPlugin = require('webpack-manifest-plugin');
const InterpolateHtmlPlugin = require('react-dev-utils/InterpolateHtmlPlugin');
const SWPrecacheWebpackPlugin = require('sw-precache-webpack-plugin');
const WorkboxWebpackPlugin = require('workbox-webpack-plugin');
const eslintFormatter = require('react-dev-utils/eslintFormatter');
const ModuleScopePlugin = require('react-dev-utils/ModuleScopePlugin');
const paths = require('./paths');
Expand Down Expand Up @@ -413,43 +413,21 @@ module.exports = {
// having to parse `index.html`.
new ManifestPlugin({
fileName: 'asset-manifest.json',
publicPath: publicPath
}),
// Generate a service worker script that will precache, and keep up to date,
// the HTML & assets that are part of the Webpack build.
new SWPrecacheWebpackPlugin({
// By default, a cache-busting query parameter is appended to requests
// used to populate the caches, to ensure the responses are fresh.
// If a URL is already hashed by Webpack, then there is no concern
// about it being stale, and the cache-busting can be skipped.
dontCacheBustUrlsMatching: /\.\w{8}\./,
filename: 'service-worker.js',
logger(message) {
if (message.indexOf('Total precache size is') === 0) {
// This message occurs for every build and is a bit too noisy.
return;
}
if (message.indexOf('Skipping static resource') === 0) {
// This message obscures real errors so we ignore it.
// https://github.com/facebook/create-react-app/issues/2612
return;
}
console.log(message);
},
minify: true,
// Don't precache sourcemaps (they're large) and build asset manifest:
staticFileGlobsIgnorePatterns: [/\.map$/, /asset-manifest\.json$/],
// `navigateFallback` and `navigateFallbackWhitelist` are disabled by default; see
// https://github.com/facebook/create-react-app/blob/master/packages/react-scripts/template/README.md#service-worker-considerations
// navigateFallback: publicUrl + '/index.html',
// navigateFallbackWhitelist: [/^(?!\/__).*/],
publicPath: publicPath,
}),
// Moment.js is an extremely popular library that bundles large locale files
// by default due to how Webpack interprets its code. This is a practical
// solution that requires the user to opt into importing specific locales.
// https://github.com/jmblog/how-to-optimize-momentjs-with-webpack
// You can remove this if you don't use Moment.js:
new webpack.IgnorePlugin(/^\.\/locale$/, /moment$/),
// Generate a service worker script that will precache, and keep up to date,
// the HTML & assets that are part of the Webpack build.
new WorkboxWebpackPlugin.GenerateSW({
exclude: [/\.map$/, /^(?:asset-)manifest.*\.js(?:on)?$/],
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add in clientsClaim: true here, which would match the default behavior in sw-precache-webpack-plugin.

Twaha-Rahman marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

On further consideration, I'd go with

exclude: [/\.map$/, /asset-manifest\.json$/]

here, matching what we were doing with sw-precache-webpack-plugin. It turns out that precaching manifest.json (which is distinct from asset-manifest.json) is valuable for some browsers, so we don't want that excluded.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another option to consider is importWorkboxFrom: 'local', so that folks end up pulling in a local copy of the Workbox runtime libraries instead of relying on the CDN (which is the default).

navigateFallback: '/index.html',
navigateFallbackWhitelist: [/^(?!\/__).*/], // fix for Firebase
Copy link
Contributor

Choose a reason for hiding this comment

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

navigateFallbackBlacklist is newly available in workbox-webpack-plugin, to address the awkwardness of needing to put in a negative pattern here.

We could also potentially get a little more nuanced, and work around some of the issues raised in #3008 (comment) (which led to navigateFallback being turned off by default) with the blacklist. One approach would be to blacklist any URLs whose last part of the path contains a . character, since that's likely to be a file extension, and those have historically been files that should not be included in the navigateFallback logic.

And example config could be:

{
  // ...other options...
  navigateFallback: '/index.html',
  navigateFallbackBlacklist: [
    new RegExp('^/_'),
    new RegExp('/[^/]+\.[^/]+$'),
  ],
}

See https://regex101.com/r/7f207Y/1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Should the . char RegExp maybe be /[^/]+\.[^/]+/?$ to include a potential trailing slash?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if there's a trailing slash, we don't want that to be blacklisted. I.e. we do want the navigateFallback behavior to take effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, in the current implementation, the firebase url is a double __ rather than single. Is that correct? If so should the single underscore prefixed urls also be blacklisted?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's safe to go with blacklisting anything that starts with /_, i.e. a single underscore. I don't know how much of a convention it is to use a single vs. a double for "special" URLs outside of Firebase, but the false-positive rate of checking for a single underscores seems likely to be acceptable.

}),
],
// Some libraries import Node modules but don't use them in the browser.
// Tell Webpack to provide empty mocks for them so importing them works.
Expand Down
4 changes: 2 additions & 2 deletions packages/react-scripts/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,14 @@
"react-dev-utils": "^5.0.0",
"style-loader": "0.19.1",
"svgr": "1.8.1",
"sw-precache-webpack-plugin": "0.11.4",
"thread-loader": "1.1.2",
"uglifyjs-webpack-plugin": "1.1.6",
"url-loader": "0.6.2",
"webpack": "3.10.0",
"webpack-dev-server": "2.11.0",
"webpack-manifest-plugin": "1.3.2",
"whatwg-fetch": "2.0.3"
"whatwg-fetch": "2.0.3",
"workbox-webpack-plugin": "^3.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

We're up to 3.6.1 now.

},
"devDependencies": {
"react": "^16.0.0",
Expand Down