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

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

merged 1 commit into from
Nov 5, 2018

Conversation

vtenfys
Copy link
Contributor

@vtenfys vtenfys commented Nov 3, 2018

  • 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 #8145

@vtenfys vtenfys requested a review from pieh November 3, 2018 15:18
@vtenfys 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 vtenfys requested review from DSchau and removed request for pieh November 5, 2018 21:09
Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

Looks good! Left a few comments/questions!

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

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

})
)
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.

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

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

@DSchau DSchau merged commit 430e8f1 into gatsbyjs:master Nov 5, 2018
@DSchau
Copy link
Contributor

DSchau commented Nov 5, 2018

Successfully published:

  • gatsby-plugin-offline@2.0.12

lipis added a commit to lipis/gatsby that referenced this pull request Nov 6, 2018
* '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)
  ...
gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants