-
-
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
Add simplified service worker invalidation #2551
Add simplified service worker invalidation #2551
Conversation
d605035
to
d2e48e8
Compare
@gaearon @tizmagik @jeffposnick you might want to double check, its based on your feedback. CI failing is unrelated to change. It is some issue with windows build and the latest version of appveyor. |
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.
With the caveat that it's unfortunate that this sort of logic needs to be implemented by each framework/starter kit, rather than being baked in to the browser, I understand why you'd want to move ahead with this brute-force approach in the interim.
I've just got the one comment about potentially only running this additional logic on localhost
instead of in production as well.
}; | ||
}; | ||
|
||
fetch(swUrl) |
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.
Throwing this out there: the overhead of this extra request would be reduced if there were an additional check that only caused it to execute when you're on localhost
or the equivalent. This PR would then solve the "zombie" SW on localhost
problem immediately, but would not attempt to unregister a service worker on a production server.
// Inside the window.onload handler:
const isLocalhost = Boolean(window.location.hostname === 'localhost' ||
// [::1] is the IPv6 localhost address.
window.location.hostname === '[::1]' ||
// 127.0.0.1/8 is considered localhost for IPv4.
window.location.hostname.match(
/^127(?:\.(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)){3}$/
)
);
if (isLocalhost) {
fetch(swUrl) // ...the rest of your logic here...
} else {
registerValidSW(swUrl); // Avoid the extra fetch() outside of localhost.
}
It's unclear how w3c/ServiceWorker#204 will get resolved, but there has been some opposition to automatically unregistering production servers on an HTTP 404 response. My hope is that the code that comes out of this PR will evolve (post-merge) to match whatever's decided upon in that issue, providing equivalent behavior prior to the actual native browser implementation.
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.
Thanks Jeff!
Do you mind scoping this for |
d2e48e8
to
aea0fee
Compare
Added the check so it only applies on local host as per @jeffposnick code.* Personally, I liked the check happening on live servers because it means that if you deployed an app with CRA and then later deployed an app with some other boilerplate, to the same domain. The user (and developer) doesn't get stuck. It also shouldn't cause a slow down, because we are instantly displaying the app regardless. Its just if there is no service worker found. We reload it right away with the SW removed. (Alt. would be to removed the SW and show the toast from #2426). However, this is a good start and would be useful to be merged in with only localhost. |
👍 |
I haven't tested but I trust this works. |
I'd appreciate if you could test the latest version and ensure it works as intended. |
Will do. I'll report back. |
Thanks! |
LGTM—I tried out both the local service worker invalidation scenario, and the |
Thanks again for checking. |
* Add service worker invalidation * Update valid service worker check only on local host
* Add service worker invalidation * Update valid service worker check only on local host
* Add service worker invalidation * Update valid service worker check only on local host
window.location.hostname === 'localhost' || | ||
// [::1] is the IPv6 localhost address. | ||
window.location.hostname === '[::1]' || | ||
// 127.0.0.1/8 is considered localhost for IPv4. |
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.
/8
?
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.
Adds invalidation of service workers based on by attempting to
fetch
service-worker.js
and if its receives a 404, or if its the wrong content type (e.g. all not found urls are being redirected to index.html or serving a 404 page without a 404 code) it unregistered the current service worker and reloads the page.This issues the issue where sites hosted on the same
url:port
(e.g.localhost:5000
) are incorrectly displayingcreate-react-app
instead of the hosted page.This was addressed in a more complex manner in #2438. However, this is a simplified version based on the feedback of @jeffposnick.
Note: I created a new PR/Branch because I believe #2438 addresses more cases, however it also adds complexity. This PR should fix most use cases when combined with #2426.