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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Nov 5, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/gatsby-plugin-offline/src/gatsby-node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))(\?.*)?$/],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

navigateFallbackBlacklist: [/\?(.+&)?no-cache=1$/],
cacheId: `gatsby-plugin-offline`,
// Don't cache-bust JS or CSS files, and anything in the static directory
Expand All @@ -101,7 +101,7 @@ exports.onPostBuild = (args, pluginOptions) => {
},
{
// Use the Network First handler for external resources
urlPattern: /^https:/,
urlPattern: /^https?:/,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

handler: `networkFirst`,
},
],
Expand Down
20 changes: 13 additions & 7 deletions packages/gatsby-plugin-offline/src/sw-append.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,19 @@ self.addEventListener(`message`, event => {
event.waitUntil(
caches.open(cacheName).then(cache =>
Promise.all(
resources.map(resource =>
cache.add(resource).catch(e => {
// ignore TypeErrors - these are usually due to
// external resources which don't allow CORS
if (!(e instanceof TypeError)) throw e
})
)
resources.map(resource => {
let request
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🎉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Some external resources don't allow
// CORS so get an opaque response
if (resource.match(/^https?:/)) {
request = fetch(resource, { mode: `no-cors` })
} else {
request = fetch(resource)
}

return request.then(response => cache.put(resource, response))
})
)
)
)
Expand Down