-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Updates to README to reflect Workbox usage. #5111
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
@@ -1833,17 +1832,6 @@ following into account: | |||
instructions for using other methods. _Be sure to always use an | |||
incognito window to avoid complications with your browser cache._ | |||
|
|||
1. If possible, configure your production environment to serve the generated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per https://developers.google.com/web/updates/2018/06/fresher-sw, I think we can just leave this section out now—I don't know how much the README needs to cater to older browsers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since service workers are only supported in evergreen browsers, I'm fine with this.
@jeffposnick Are you proposing we merge #4169 as is and then apply these changes after that? |
[`runtimeCaching`](https://github.com/GoogleChrome/sw-precache#runtimecaching-arrayobject) | ||
option in the `SWPrecacheWebpackPlugin` section of | ||
[`webpack.config.prod.js`](../config/webpack.config.prod.js). | ||
runtime caching strategy for those requests, you can add in a [`runtimeCaching`](https://developers.google.com/web/tools/workbox/modules/workbox-webpack-plugin#generateSW-runtimeCaching) to section to a custom `workbox.config.js` file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workbox.config.js
file is something that was alluded to, but I believe it's just hypothetical right now.
I'm assuming that that functionality is going to be added to the next
branch at some point by the c-r-a
maintainers, and this reference to workbox.config.js
could at that point be linked to whatever documentation is written for that new functionality.
(If I'm getting those assumptions wrong, let me know!)
Right, once #4169 is merged and things switch over to Workbox, there are some parts of the README that would need to be updated, and that's included in this PR. (#4169 is not my PR, so I can't just add these changes into that.) There are also a couple of things that have changed in terms of browser functionality since #3864 was last addressed, and this PR reflects that. |
Can you please remove the references to configuration for now? No one has championed a PR for this yet. |
Merging, cla: #3924 (comment) |
* Updates to README to reflect Workbox usage. * Update README.md * Update README.md
R: @gaearon @Timer
This is a continuation of the fix for #3864, with additional changes to reflect the usage of Workbox and the fact that setting proper
Cache-Control
headers on the generated service worker file should (hopefully!) no longer be a concern for modern web browsers.This PR depends on #4169 being merged.