fix(gatsby-plugin-offline): Serve the offline shell for short URLs + use no-cors for external resources #9679

vtenfys · 2018-11-03T11:43:41Z

Fixes the fallback whitelist regex so that short pages match (e.g. /), so that these are served offline correctly
Uses no-cors mode when caching external resources, so that errors don't appear if the target doesn't allow CORS requests
Allow insecure external resources to be cached for testing on localhost

DSchau

Looks good! Left a few comments/questions!

DSchau · 2018-11-05T21:10:15Z

packages/gatsby-plugin-offline/src/gatsby-node.js

@@ -88,7 +88,7 @@ exports.onPostBuild = (args, pluginOptions) => {
    // URLs and not any files hosted on the site.
    //
    // Regex based on http://stackoverflow.com/a/18017805
-    navigateFallbackWhitelist: [/^[^?]*([^.?]{5}|\.html)(\?.*)?$/],
+    navigateFallbackWhitelist: [/^([^.?]*|[^?]*\.([^.?]{5,}|html))(\?.*)?$/],


This is getting hard to follow 😱 (not your fault!)

So originally this was for capturing a file name without an extension, so if we break this apart

start of string

PARTS

[^.?]* matches anything without a dot, e.g. /asdf

[^?]*\.([^.?]{5,}|html) matches also html files?

(\?.*) matches query params

end of string

Did I encapsulate that OK?

Almost, but the middle is a little bit more complex:

start of string

PARTS

[^.?]* matches anything without a dot or question mark, e.g. /asdf

[^?]*\.([^.?]{5,}|html) matches anything without an extension, or with the extension html, where an extension is considered to be a dot followed by up to 4 characters (so 5 characters is not considered an extension)

(\?.*) matches query params

end of string

DSchau · 2018-11-05T21:11:08Z

packages/gatsby-plugin-offline/src/gatsby-node.js

@@ -101,7 +101,7 @@ exports.onPostBuild = (args, pluginOptions) => {
      },
      {
        // Use the Network First handler for external resources
-        urlPattern: /^https:/,
+        urlPattern: /^https?:/,


Is this a best practice? This seems significant in that we'll cache non-secure resources now, too.

Ah, got it!

Allow insecure external resources to be cached for testing on localhost

Is there a risk here of content being cached in localhost and leading to confusion? I'd almost prefer it kept not working in localhost, but perhaps that's a naive statement?

Content is already cached on localhost to allow for testing - this is just to make it work properly for external resources which use // (e.g. the Google Fonts plugin) which are served over HTTP due to matching protocol

DSchau · 2018-11-05T21:18:40Z

packages/gatsby-plugin-offline/src/sw-append.js

-            })
-          )
+          resources.map(resource => {
+            let request


Few questions here:

Do we still want to throw on (certain?) errors? I suppose the promise will just reject?

This seems weird that we've totally changed the approach, i.e. we're fetching everything now?

This seems to fix things like google-analytics. Were you able to validate that?

Just asking these to make sure we're on the same page and I understand things!

Do we still want to throw on (certain?) errors? I suppose the promise will just reject?

If there are any odd errors then yes, the promise will reject the same way as before

This seems weird that we've totally changed the approach, i.e. we're fetching everything now?

We were already fetching everything with the old code, just never with no-cors, so sometimes there would be console errors

This seems to fix things like google-analytics. Were you able to validate that?

I haven't tested Google Analytics explicitly but yes, it should be fixed since no-cors means it can be cached. I've tested other external resources though to check it's okay.

Maybe we validate with that one just to validate we've fixed the issue this is fixing?

Once that's done, I think this will be good to go and merge 🎉

This is not a best practice and will quickly lead to improper caching of resources.

From Workbox's own documentation:

Beware of opaque responses!

A common source of unexpectedly high quota usage is due to runtime caching of opaque responses, which is to say, cross-origin responses to requests made without CORS enabled.

Browsers automatically inflate the quota impact of those opaque responses as a security consideration. In Chrome, for instance, even an opaque response of a few kilobytes will end up contributing around 7 megabytes towards your quota usage.

This should probably be enabled on a case-by-case basis using the plugin's options or not at all. If the external domain allows CORS, the resource should flag it using the HTML crossorigin attribute.

@wconnorwalsh awesome; thank you for the info! This change also (could have) illustrated exactly what you reference here:

A common source of unexpectedly high quota usage is due to runtime caching of opaque responses

as we've seen some recent weirdness with gatsbyjs.org.

I'm mostly out today, would you be interested in reverting this change and making this opt-in rather than enabled by default? If not, we'll get to it as soon as we can!

cc @wconnorwalsh @DSchau please note that only the changes to sw-append.js should be reverted - the others are unrelated!

I've looked into this a bit more and going to create a PR shortly to revert this + fix a few more things.

DSchau

@davidbailey00 validated with the google analytics plugin and we're good to go. 🎉

DSchau · 2018-11-05T21:54:51Z

Successfully published:

gatsby-plugin-offline@2.0.12

* 'master' of github.com:gatsbyjs/gatsby: (63 commits) Update how-to-contribute.md to mention the style guide when writing blogs/tutorials (gatsbyjs#9742) Added Tylermcginnis website (gatsbyjs#9619) Fix grammar and punctuation (gatsbyjs#9498) Fix typo of plugin authoring (gatsbyjs#9737) Authentication tutorial - small fixes (gatsbyjs#9738) chore: move run-sift (gatsbyjs#9549) docs: fix minor typo (gatsbyjs#9730) chore(release): Publish fix(gatsby-plugin-page-creator): ensure that __tests__ directory is actually ignored (gatsbyjs#9720) fix: revert admin redirect (gatsbyjs#9728) fix: adjust page order to make nested matchPaths work (gatsbyjs#9719) feat(gatsby-plugin-sharp): cache base64 if possible (gatsbyjs#9059) chore(release): Publish fix(gatsby-plugin-offline): Serve the offline shell for short URLs + use no-cors for external resources (gatsbyjs#9679) chore(release): Publish fix: ensure babel-preset-gatsby can be used with unit tests (gatsbyjs#9629) feat(www): Filter posts by date (gatsbyjs#9400) fix(blog): azure blog post url date (gatsbyjs#9718) feat(blog): Add post on publishing to Azure (gatsbyjs#8868) Emphasize importance of promise return on source-plugin docs (gatsbyjs#9650) ...

…use no-cors for external resources (gatsbyjs#9679) - Fixes the fallback whitelist regex so that short pages match (e.g. `/`), so that these are served offline correctly - Uses `no-cors` mode when caching external resources, so that errors don't appear if the target doesn't allow CORS requests - Allow insecure external resources to be cached for testing on `localhost` Fixes gatsbyjs#8145

fix(gatsby-plugin-offline): Use no-cors for external resources

7ba8ec5

vtenfys requested a review from pieh November 3, 2018 15:18

vtenfys changed the title ~~fix(gatsby-plugin-offline): Use no-cors mode when caching external resources~~ fix(gatsby-plugin-offline): Serve the offline shell for short URLs + use no-cors for external resources Nov 5, 2018

vtenfys requested review from DSchau and removed request for pieh November 5, 2018 21:09

DSchau reviewed Nov 5, 2018

View reviewed changes

DSchau approved these changes Nov 5, 2018

View reviewed changes

DSchau merged commit 430e8f1 into gatsbyjs:master Nov 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(gatsby-plugin-offline): Serve the offline shell for short URLs + use no-cors for external resources #9679

fix(gatsby-plugin-offline): Serve the offline shell for short URLs + use no-cors for external resources #9679

vtenfys commented Nov 3, 2018 •

edited

Loading

DSchau left a comment

DSchau Nov 5, 2018

vtenfys Nov 5, 2018

DSchau Nov 5, 2018

DSchau Nov 5, 2018

vtenfys Nov 5, 2018

DSchau Nov 5, 2018

vtenfys Nov 5, 2018

DSchau Nov 5, 2018

wconnorwalsh Nov 7, 2018

DSchau Nov 7, 2018

vtenfys Nov 8, 2018

vtenfys Nov 14, 2018

DSchau left a comment

DSchau commented Nov 5, 2018

fix(gatsby-plugin-offline): Serve the offline shell for short URLs + use no-cors for external resources #9679

fix(gatsby-plugin-offline): Serve the offline shell for short URLs + use no-cors for external resources #9679

Conversation

vtenfys commented Nov 3, 2018 • edited Loading

DSchau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Beware of opaque responses!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DSchau left a comment

Choose a reason for hiding this comment

DSchau commented Nov 5, 2018

vtenfys commented Nov 3, 2018 •

edited

Loading