Disable sw-precache plugin's navigateFallback #3024

chee · 2017-08-28T23:35:27Z

facebook-github-bot · 2017-08-28T23:35:32Z

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!

facebook-github-bot · 2017-08-28T23:48:37Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

gaearon · 2017-08-29T01:34:36Z

packages/react-scripts/config/webpack.config.prod.js

@@ -343,6 +343,10 @@ module.exports = {
      navigateFallbackWhitelist: [/^(?!\/__).*/],
      // Don't precache sourcemaps (they're large) and build asset manifest:
      staticFileGlobsIgnorePatterns: [/\.map$/, /asset-manifest\.json$/],
+      // Precache files in public/, and allow them to be accessed directly
+      staticFileGlobs: ['./public/**/*'],


What is it resolving this path against?

i've swapped this out for paths.appPublic in the latest commit

viankakrisna · 2017-08-29T12:45:00Z

packages/react-scripts/config/webpack.config.prod.js

@@ -343,6 +343,10 @@ module.exports = {
      navigateFallbackWhitelist: [/^(?!\/__).*/],
      // Don't precache sourcemaps (they're large) and build asset manifest:
      staticFileGlobsIgnorePatterns: [/\.map$/, /asset-manifest\.json$/],
+      // Precache files in public/, and allow them to be accessed directly
+      staticFileGlobs: [paths.appPublic + '/**/*'],


can we whitelist the extension here? I usually have .htaccess and index.php in public folder, will it also download these files?

the .htaccess will not, because it won't be picked up by the glob.

it will try to download the index.php, the result of which will depend on the configuration of your server.

this highlights something that might surprise a user just as much as them being unable to access their public/ files:

anything you put in public/ will now be precached during the first load.

depending on what people are doing in their public/ folder, this could have alarming consequences.

in my own use case i've favoured navigate ignoring requests which appear to have file extensions.

but this is not ideal either, because we can't know what routes the user wants to handle in their app.

i think the least surprising behaviour would be for everything in public/ to be ignored by navigateFallback.

the configuration for navigateFallback is, however, an array of RegExps called navigateFallbackWhitelist

the code would be bizarre, and could result in quite a long list of RegExps for the service worker to match against for every request.

like, you'd be setting the whitelist to something like:

navigateFallbackWhitelist: filesInPublic.map(path => RegExp('^(?!' + path + ').*$') )

which is not ideal.

the dream would be if there were a navigateFallbackBlacklist.

@jeffposnick mentioned here that that was planned for a future library

(actually that wouldn't even work because it is checked with whitelist.some. so everything would pass one of the regexen.)

this might:

navigateFallbackWhitelist: [ '(?!' + filesInPublic.join('|') + ').*$' ]

though it would still falsly match lol.md5 if there was a file called lol.md in public.

not ideal.

negative regex is hard

how about just whitelist it for now?

staticFileGlobs: ['js', 'css', 'json', 'html', 'svg', 'otf', 'ttf'].map(ext => paths.appPublic + '/**/*.' + ext),

will it navigate correctly?

ah, the non-whitelisted extensions would still not be able to be accessed... navigateFallbackBlacklist would be ideal...

plus we'd be making a decision as to what files would be accessible and viewable in browser for the user, which may be quite surprising. (ie: how come i could visit my data.json but not my data.txt, how come view image in new tab doesn't work for .gifs)

@jeffposnick would you consider a PR on sw-precache adding a navigateFallbackBlacklist, with a default of [] that returned early if url matches any of its items?

however, switching to such a blacklist would mean there'd be no way of doing what this PR would do in its current state, which is getting something into precache that isn't required in the js.

@viankakrisna what is your expected behaviour for that index.php in public/? it surely isn't currently working once the service worker has installed?

@chee mainly for injecting initial data and user agent redirects. i want to send server generated markup for bots. It would still work for initial request and bots (afaik no bots will install sw right?).

ah that makes sense.

so if there was a fallback blacklist then it would behave the same as it does now for you, but with this pull request a user would be needlessly caching the result of your index.php running

Yeah, and i thought it would cache the contents of index.php. Thanks for clearing that up!

chee · 2017-08-29T22:37:02Z

i'm going to leave this open as it was what @jeffposnick suggested, but i'm personally going to favour building service-worker.js myself with the sw-precache cli tool during my build process, as it looks like i'll need plenty more control than i can fairly ask for from cra.

this way i don't have to eject but i still get to control this part of my application.

there's something to be said for writing about this in the README.md, as it looks like the desire for more control of the service worker is a considerable force in the issues and pull requests open now on this repository, and it only takes a few moments to set it up.

it's optional configuration without ejection if the defaults don't fit your application's needs, and is more in line with adding further code or libraries to your application than abandoning the simplicity of create-react-app

goldhand

This looks good to me. Sorry for the delay @chee

jeffposnick · 2017-09-05T14:23:35Z

Using staticFileGlobs : paths.appPublic + '/**/*.html could be a reasonable compromise here. The thing to keep in mind is that we only need to match URLs that will be used as navigation targets, and it's probably a reasonable assumption that anything ending in .html will be used as a navigation target, while things ending in .json, .js, etc. won't be.

While testing out the service worker behavior, you'll often find yourselves navigation to a .json URL, but there's not how actual users of your application will interact with those URLs. When a .json/.js/etc. URL is referenced as a resource from a page (as opposed to being used as a navigation target), it will automatically be exempted from the navigateFallback behavior, so there's no need to accommodate them explicitly.

chee · 2017-09-05T18:46:16Z

actual users may well right click and try to view an image in a new tab, however, which is disabled in a surprising manner by this (unless the image was included in the javascript, which maybe that's the only supported way).

with my blog (which was from where i came across this), i let people look at the original markdown source of my posts by appending .md to the end of the url of any post they are reading.
i realise it's an unusual use-case, but it is a use-case.

i don't feel entirely comfortable with making the presumption that users probably don't want to navigate to most of the files in public/, but if everyone else is entirely comfortable with that presumption i will make that change on this pull request.

Beware that files in /public are redirected by service worker to index.html Means that https://webhooks.integrations.costlocker.com/favicon.ico is accessible on in anonymous window :) facebook/create-react-app#2894 (comment) facebook/create-react-app#3024 (comment)

https://fakturoid.integrations.costlocker.com/api/redirect?project=52375&billing=NEW&amount=78600 https://user-images.githubusercontent.com/7994022/30257989-ed274944-96b5-11e7-99b2-34274efd01c9.png service worker swallows requests... facebook/create-react-app#2894 (comment) facebook/create-react-app#3024 (comment)

jeffposnick · 2017-09-18T17:01:26Z

I feel like the staticFileGlobs : paths.appPublic + '/**/*.html configuration is a good compromise between caching URLs that are most likely to be navigation targets while not being too aggressive about precaching all of the resources in public/.

It might be a good idea to move forward with that and then, if needed, adjust the pattern to be more or less restrictive if there's feedback post-deployment?

react-scripts-dangerous · 2017-09-18T17:41:59Z

Hello! I'm a bot that helps facilitate testing pull requests.

Your pull request (commit 60145f9) has been released on npm for testing purposes.

npm i react-scripts-dangerous@1.0.13-60145f9.0
# or
yarn add react-scripts-dangerous@1.0.13-60145f9.0
# or
create-react-app --scripts-version=react-scripts-dangerous@1.0.13-60145f9.0 folder/

Note that the package has not been reviewed or vetted by the maintainers. Only install it at your own risk!

Thanks for your contribution!

chee · 2017-09-19T10:22:00Z

@jeffposnick i'm with you on that. here's a commit

react-scripts-dangerous · 2017-09-19T11:42:06Z

Hello! I'm a bot that helps facilitate testing pull requests.

Your pull request (commit f2b3fa3) has been released on npm for testing purposes.

npm i react-scripts-dangerous@1.0.13-f2b3fa3.0
# or
yarn add react-scripts-dangerous@1.0.13-f2b3fa3.0
# or
create-react-app --scripts-version=react-scripts-dangerous@1.0.13-f2b3fa3.0 folder/

Note that the package has not been reviewed or vetted by the maintainers. Only install it at your own risk!

Thanks for your contribution!

jeffposnick · 2017-09-19T14:35:24Z

This PR looks 👍

Thanks for taking it on, @chee!

Quick ping to @Timer & @gaearon or anyone else who could merge, assuming their fine with the approach.

Timer · 2017-09-19T15:21:44Z

Just to be clear here, this changes the behavior to only cache html files in public/ instead of all files? It seems this PR has changed since it was open and I just want to make sure I have a good understanding.

Do requests for other files in public/ go to the network?

jeffposnick · 2017-09-19T16:01:55Z

This change will lead to only .html files that live in public/ being precached.

Navigating to any of those .html files will result in the cached HTML specific to the URL being displayed.

Requesting any of the other files in public/ as subresources on a page will result in those requests being fulfilled without any service worker involvement.

(The one wrinkle here is that if someone navigates directly to a URL corresponding to a non-.html file in public/, like navigating to a .json URL, that request will still be fulfilled by the service worker with the contents of index.html. This is because the service worker treats navigations differently, and assumes that a navigation request for a URL isn't precached is for a Single Page App-style URL that can be fulfilled by index.html + client-side routing.)

chee · 2017-09-19T17:27:58Z

@Timer as @jeffposnick says: non-.html files will continue to be intercepted by the service-worker if they are navigated to, but non-navigation requests to them will work as normal and expected.

i think the behaviour is a little surprising, but after quite a bit of discussion i can't see any simple universal approach that isn't surprising.

the least surprising would probably be for the items in public/ to be added to a blacklist of paths the service-worker wouldn't intercept on navigate.

without such a blacklist available at present, this approach, while still full of surprises (like if someone right clicks a non-precached image from public/ and tries to view it in a new tab), might at least be the most useful. even that is, like, conjecture though.

it also, i'm now noting, doesn't really close or fix the two issues mentioned in the initial pr description (#3008, #2894) anymore, so i've removed that line from the text.

jeffposnick · 2017-09-19T17:38:35Z

the least surprising would probably be for the items in public/ to be added to a blacklist of paths the service-worker wouldn't intercept on navigate.

The flip side is that if you use create-react-app with the understanding that it will make your web app work while offline, and static HTML files that you add to public/ aren't accessible while offline (which they wouldn't be, without this PR), that's also surprising.

As @chee says, determining what's least surprising is something of a judgment call.

chee · 2017-09-22T11:19:13Z

@jeffposnick that's so true

it's very subjective.

here's a new issue where it was suprising again but different: #3174

that wouldn't be fixed by this PR but would be solved by the previous version which cached everything.

and it wouldn't be fixed at all by my preference of blacklisting everything in public/

v subjective.

viankakrisna · 2017-09-22T11:38:46Z

(The one wrinkle here is that if someone navigates directly to a URL corresponding to a non-.html file in public/, like navigating to a .json URL, that request will still be fulfilled by the service worker with the contents of index.html. This is because the service worker treats navigations differently, and assumes that a navigation request for a URL isn't precached is for a Single Page App-style URL that can be fulfilled by index.html + client-side routing.)

does this also true for requests using fetch / xhr?

chee · 2017-09-22T13:16:28Z

@viankakrisna fetch/xhr will work as expected with no surprises because they aren't a navigate. it's only truly visiting it in the browser that will be slurped up by sw

@jeffposnick i'm not missing any other cases am i?

chee · 2017-09-22T23:25:08Z

here is another example for somebody surprised by the current behaviour #3171

none of the solutions come up with so far would help them, other than a configurable whitelist/blacklist.

jeffposnick · 2017-10-04T17:21:36Z

I'm starting to think that ditching navigateFallback by default would get might just be the best path forward. See #2398 (comment)

chee · 2017-10-04T17:53:03Z

@jeffposnick that seems pretty good.

maybe some documentation would be in order, or a blog post, for how uncomplicated it is to create an sw-precache service worker to suit your needs without the need to eject.

for now i am going to update this pull request to disable navigateFallback altogether.

i will also rename the PR, and put the references to #3008, #2894 back into the opening post because it goes back to fixing them again once that change is made.

chee · 2017-10-04T17:54:07Z

@goldhand how do you feel about the approach of disabling navigateFallback altogether? -> #2398 (comment)
?

chee · 2017-10-04T17:58:09Z

@jeffposnick do you think we should still precache their .html files? i can see it being useful but it also strikes me as bizarre and arbitrary

react-scripts-dangerous · 2017-10-04T19:21:58Z

Hello! I'm a bot that helps facilitate testing pull requests.

Your pull request (commit 30ba746) has been released on npm for testing purposes.

npm i react-scripts-dangerous@1.0.13-30ba746.0
# or
yarn add react-scripts-dangerous@1.0.13-30ba746.0
# or
create-react-app --scripts-version=react-scripts-dangerous@1.0.13-30ba746.0 folder/

Note that the package has not been reviewed or vetted by the maintainers. Only install it at your own risk!

Thanks for your contribution!

chee · 2017-10-04T22:56:44Z

thanks robot

jeffposnick · 2017-10-06T13:39:05Z

Thanks for continuing to work on this, @chee! FWIW, I wanted to explicitly discuss this with the relevant maintainers, as it's a breaking change for a subset of users. See #3248.

chee · 2017-11-06T21:56:52Z

this has been obsoleted by @jeffposnick's pull request at #3419

thank you for your support and patience

have the service-worker precache files in public/

78cf568

facebook-github-bot added the CLA Signed label Aug 28, 2017

gaearon reviewed Aug 29, 2017

View reviewed changes

use appPublic from paths for staticFileGlobs

60145f9

viankakrisna reviewed Aug 29, 2017

View reviewed changes

goldhand approved these changes Aug 31, 2017

View reviewed changes

chee mentioned this pull request Sep 1, 2017

service worker swallows requests for files added to public/ #3008

Closed

precache only html files in public/

f2b3fa3

jeffposnick mentioned this pull request Sep 19, 2017

Pre-rendering with react-snapshot and service worker incorrect behavior #3132

Closed

facebook deleted a comment from react-scripts-dangerous Sep 19, 2017

chee changed the title ~~precache files in public/ with service-worker~~ Disable sw-precache plugin's navigateFallback Oct 4, 2017

disable sw-precache's navigateFallback

30ba746

jeffposnick mentioned this pull request Oct 6, 2017

Remove 'navigateFallback' behavior from default SW config #3248

Closed

chee mentioned this pull request Nov 6, 2017

Remove the navigateFallback behavior from the generated service worker #3419

Merged

chee closed this Nov 6, 2017

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

Disable sw-precache plugin's navigateFallback #3024

Disable sw-precache plugin's navigateFallback #3024

Conversation

chee commented Aug 28, 2017 • edited Loading

facebook-github-bot commented Aug 28, 2017

facebook-github-bot commented Aug 28, 2017

gaearon Aug 29, 2017

Choose a reason for hiding this comment

chee Aug 29, 2017

Choose a reason for hiding this comment

viankakrisna Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

chee Aug 29, 2017

Choose a reason for hiding this comment

chee Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

chee Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

viankakrisna Aug 29, 2017

Choose a reason for hiding this comment

viankakrisna Aug 29, 2017

Choose a reason for hiding this comment

chee Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

viankakrisna Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

chee Aug 30, 2017

Choose a reason for hiding this comment

viankakrisna Aug 30, 2017

Choose a reason for hiding this comment

chee commented Aug 29, 2017

goldhand left a comment

Choose a reason for hiding this comment

jeffposnick commented Sep 5, 2017

chee commented Sep 5, 2017 • edited Loading

jeffposnick commented Sep 18, 2017

react-scripts-dangerous commented Sep 18, 2017

chee commented Sep 19, 2017

react-scripts-dangerous commented Sep 19, 2017

jeffposnick commented Sep 19, 2017

Timer commented Sep 19, 2017

jeffposnick commented Sep 19, 2017

chee commented Sep 19, 2017

jeffposnick commented Sep 19, 2017

chee commented Sep 22, 2017 • edited Loading

viankakrisna commented Sep 22, 2017

chee commented Sep 22, 2017 • edited Loading

chee commented Sep 22, 2017 • edited Loading

jeffposnick commented Oct 4, 2017

chee commented Oct 4, 2017

chee commented Oct 4, 2017

chee commented Oct 4, 2017

react-scripts-dangerous commented Oct 4, 2017

chee commented Oct 4, 2017

jeffposnick commented Oct 6, 2017

chee commented Nov 6, 2017

chee commented Aug 28, 2017 •

edited

Loading

viankakrisna Aug 29, 2017 •

edited

Loading

chee Aug 29, 2017 •

edited

Loading

chee Aug 29, 2017 •

edited

Loading

chee Aug 29, 2017 •

edited

Loading

viankakrisna Aug 30, 2017 •

edited

Loading

chee commented Sep 5, 2017 •

edited

Loading

chee commented Sep 22, 2017 •

edited

Loading

chee commented Sep 22, 2017 •

edited

Loading

chee commented Sep 22, 2017 •

edited

Loading