Remove the navigateFallback behavior from the generated service worker

[jeffposnick]

Fixes#3248
Fixes#3008

I'd consider this is a breaking change for folks who relied on the old default behavior, so rolling this out as part of an appropriately versioned release of create-react-app would make sense.

CC: @gaearon@Timer@addyosmani@chee

[facebook-github-bot]

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!

[chee]

this seems to make my pull request at #3024 obsolete

it contains the same change but with better documentation.

i will close my pr

[piotr-cz]

There should be some way to keep current behavior as after upgrading CRA existing apps will break in offline mode and there would be no way to fix them - perhaps global switch in package.json.

Other solution is to keep current behavior and add an exception - for example for API calls, like in #3029.

[piotr-cz]

Another solution would be to provide fallbacks only if the accept header is text/html, just like in the Proxy API Requests in Development

[Timer]

I believe this is a good trade-off, thanks for the excellent explanations @jeffposnick.

[jeffposnick]

@piotr-cz, every modern browser sends text/html as part of the Accept: header for navigation requests: https://developer.mozilla.org/en-US/docs/Web/HTTP/Content_negotiation/List_of_default_Accept_values#Default_values

[piotr-cz]

@jeffposnick My idea is to differentiate navigation requests (accept: text/html) from API requests. For example when application adds a header accept: application/json for every API request

[jeffposnick]

@piotr-cz: API and all other subresource requests were already being differentiated, by virtue of checking for event.request.mode === 'navigate' inside the service worker's fetch handler, and only triggering the navigateFallback behavior when that was true.

This differentiation wasn't sufficient to prevent confusion for a subset of developers, for all the reasons noted in the various bugs linked to from this PR.

[seejamescode]

If we can’t expose customization of the SWPrecacheWebpackPlugin configs without ejecting, I think this is the best default behavior. The biggest benefit I get from Create-React-App is keeping the compilation step of my front-end up-to-date for a Node app. However, that doesn’t work if our API endpoints are being blocked by the service worker.

Would this be considered a breaking change if it only breaks offline 404s?

[piotr-cz]

@seejamescode Are you sure the API endpoints are blocked by service worker?
It should take over only requests with accept: text/html.

[seejamescode]

Sorry for vagueness earlier. The particular situation is that my homepage has an anchor link to an Express get request using text/html. I use this request to do a Passport authenticate for the Twitter API. I can dig deeper, but is there another type of request I can call that oAuth work with?

[piotr-cz]

Looks like there must be at least one navigateFallback path exclusion for implementing oAuth.
#3029 should solve it.

[Timer]

[piotr-cz]

Now that service worker registration is disabled by default by #3817, wouldn't it make sense to revert this commit?

I'd say that developers who opt in to use service workers would also expect the app to work offline when using History API like React Router or as receiver of Web Share API URLs

[jeffposnick]

Accommodating all deployment scenarios is still tricky, but yes, I'd imagine that once enabling service workers is opt-in, the developers who do choose to enable them are more likely to benefit from navigateFallback behavior, and are less likely to be caught by surprise.

So I could see the rationale behind reverting this PR for the next major release.

CC: @gaearon@Timer

[Timer]

Can we open a new issue to talk about this please?

[piotr-cz]

@jeffposnick
Both this PR and #3817 were merged to the next branch.

@Timer
Created #3870