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

Remove 'navigateFallback' behavior from default SW config #3248

Closed
jeffposnick opened this issue Oct 6, 2017 · 6 comments
Closed

Remove 'navigateFallback' behavior from default SW config #3248

jeffposnick opened this issue Oct 6, 2017 · 6 comments

Comments

@jeffposnick
Copy link
Contributor

I wanted to move my latest comments from #2398 (comment) into a specific issue to discuss them separate from the larger meta-conversation around service workers in create-react-app:

Here's one possible change that could be made short of removing service worker generation: by default, index.html is used to handle all navigation requests for URLs that aren't precached (config).

This behavior is in there to support SPA-style offline usage, where users might navigate to URLs that don't correspond to underlying HTML files. That being said, many developers don't fall into that category.

What we could do is remove navigateFallback/navigateFallbackWhitelist completely from the default configuration. Offline usage for index.html and any other files that exist as part of the Webpack build will still work. Offline usage for SPA-style routing will continue to work if hash-based routing is used (as opposed to pushState routing).

The drawback is that getting offline support with pushState routing working would require an eject and re-adding navigateFallback to the config, but at least there's a specific place in the existing documentation for this use case, where we can clearly spell out the changes that are needed.

Given that it would break the "navigate to a random URL and expect it to work" offline experience for any SPAs that have gone through the steps to support the History API's pushState(), I think that if we move forward, it needs to wait until the next major release of create-react-app.

@chee has put together #3024 which would be sufficient in terms of a code change, but we'd need to accompany it with changes to the docs explaining how to eject and re-enable that behavior for SPAs using the History API (which I'm happy to write).

CC: @Timer @gaearon @addyosmani for their thoughts.

@gaearon
Copy link
Contributor

gaearon commented Nov 3, 2017

The drawback is that getting offline support with pushState routing working would require an eject and re-adding navigateFallback to the config

That sounds like a deal-breaker, no? We expect that the vast majority of apps are built with client side pushState routing, not hash based one. If anything, I expect people want it more than service workers.

If the default experience is broken, it seems like the majority would be better served by disabling service workers than by breaking client-side routing out of the box.

Do I misunderstand the proposal?

@jeffposnick
Copy link
Contributor Author

Your understanding of the proposal sounds correct, though I want to clarify what broken means in this context.

Removing navigateFallback would mean that apps using client-side pushState routing would require a network connection to fulfill the initial navigation to a non-/ or /index.html URL.

The offline experience for those pushState()ed URLs would be broken, but navigations to those URLs would work as expected under this proposal whenever there is a network connection.

The benefit of going with this approach vs. disabling service workers entirely is that navigations to /index.html would still work while offline under all circumstances, and that's the entry point for anything added to an Android device's homescreen.

If folks are not a fan of this proposal, then earlier proposals, like #3024 (comment) are still viable ways addressing some of the same pain points that this tries to solve.

@gaearon
Copy link
Contributor

gaearon commented Nov 3, 2017

Okay, this sounds reasonable. Thanks for explaining!

@jeffposnick
Copy link
Contributor Author

I've put together #3419 to update the config + docs.

@chee, are you cool with closing #3024 in favor of my PR? I didn't want to put the burden on updating the docs and comments in your PR.

@chee
Copy link

chee commented Nov 6, 2017

i wouldn’t have minded the burden! i love burden

but it’s now closed in favour of yours

thanks! it looks like a good move.

@gaearon
Copy link
Contributor

gaearon commented Jan 10, 2018

Fixed by #3419.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants