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 simplified service worker invalidation #2551

Merged
merged 2 commits into from Jun 27, 2017

Conversation

Projects
None yet
6 participants
@ro-savage
Copy link
Contributor

commented Jun 17, 2017

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.

@ro-savage ro-savage force-pushed the ro-savage:simplified-sw-invalidation branch from d605035 to d2e48e8 Jun 17, 2017

@ro-savage

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2017

@gaearon @tizmagik
This is ready for review and merge. Should not cause any breaking changes.

@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.

@jeffposnick
Copy link
Contributor

left a comment

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)

This comment has been minimized.

Copy link
@jeffposnick

jeffposnick Jun 20, 2017

Contributor

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.

This comment has been minimized.

Copy link
@ro-savage

ro-savage Jun 27, 2017

Author Contributor

Thanks Jeff!

@gaearon

This comment has been minimized.

Copy link
Member

commented Jun 26, 2017

@ro-savage

Do you mind scoping this for localhost for now, as suggested by @jeffposnick?

@gaearon gaearon added this to the 1.0.x milestone Jun 26, 2017

@ro-savage ro-savage force-pushed the ro-savage:simplified-sw-invalidation branch from d2e48e8 to aea0fee Jun 27, 2017

@ro-savage

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2017

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.

@jeffposnick

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2017

👍

@gaearon

This comment has been minimized.

Copy link
Member

commented Jun 27, 2017

I haven't tested but I trust this works.
However longer term this is not very sustainable. Filed #2638.

@gaearon gaearon merged commit 78dbf7b into facebook:master Jun 27, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gaearon gaearon referenced this pull request Jun 28, 2017

Merged

Changelog for 1.0.8 #2664

@gaearon

This comment has been minimized.

Copy link
Member

commented Jun 28, 2017

@jeffposnick

I'd appreciate if you could test the latest version and ensure it works as intended.
https://github.com/facebookincubator/create-react-app/releases/tag/v1.0.8

@jeffposnick

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2017

Will do. I'll report back.

@gaearon

This comment has been minimized.

Copy link
Member

commented Jun 28, 2017

Thanks!

@jeffposnick

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2017

LGTM—I tried out both the local service worker invalidation scenario, and the PUBLIC_URL is set to a cross-origin scenario, and the PRs to deal with both seem to be working as expected.

@gaearon

This comment has been minimized.

Copy link
Member

commented Jun 28, 2017

Thanks again for checking.

romaindso added a commit to romaindso/create-react-app that referenced this pull request Jul 10, 2017

Add simplified service worker invalidation (facebook#2551)
* Add service worker invalidation

* Update valid service worker check only on local host

wmonk referenced this pull request in wmonk/create-react-app-typescript Aug 7, 2017

Add simplified service worker invalidation (#2551)
* Add service worker invalidation

* Update valid service worker check only on local host

morgs32 added a commit to BrickworkSoftware/create-react-app that referenced this pull request Sep 1, 2017

Add simplified service worker invalidation (facebook#2551)
* 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.

This comment has been minimized.

Copy link
@alexmadjar

This comment has been minimized.

@lock lock bot locked and limited conversation to collaborators Jan 20, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.