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

Offline plugin sometimes has a fetch error #8145

Closed
m-allanson opened this issue Sep 14, 2018 · 8 comments · Fixed by #8183
Closed

Offline plugin sometimes has a fetch error #8145

m-allanson opened this issue Sep 14, 2018 · 8 comments · Fixed by #8183

Comments

@m-allanson
Copy link
Contributor

m-allanson commented Sep 14, 2018

Description

Some combinations of plugins can cause console errors in the browser when first visiting a site.

Using gatsby-plugin-offline with gatsby-plugin-google-analytics and a plugin that does something like:

exports.onRenderBody = ({ setHeadComponents }) => {
    setHeadComponents([<script />])
}

will trigger the following browser error when first visiting the site:

Access to fetch at 'https://www.google-analytics.com/analytics.js' from origin 'http://localhost:9001' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource. If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.
(index):1 Uncaught (in promise) TypeError: Failed to fetch

Reloading the page will load the page without errors. Removing the service worker and reloading the page will display the error again.

In short:

  • offline plugin + analytics plugin = no error
  • offline plugin + analytics plugin + another script = analytics fetch error

Steps to reproduce

Repo: https://github.com/m-allanson/offline-analytics
Demo video: https://www.youtube.com/watch?v=zPzEq7aMGfA

  • clone the repo
  • yarn
  • yarn run build
  • gatsby serve
  • browse to http://localhost:9000
  • in Chrome open the application tab of the developer tools
  • ensure unregister service workers is checked
  • click clear site data
  • reload the page
  • see console errors in the browser

Expected result

No console error

Actual result

Console error on first visit

Environment

  System:
    OS: macOS High Sierra 10.13.6
    CPU: x64 Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz
    Shell: 3.2.57 - /bin/bash
  Binaries:
    Node: 10.9.0 - ~/.nvm/versions/node/v10.9.0/bin/node
    Yarn: 1.9.4 - /usr/local/bin/yarn
    npm: 6.2.0 - ~/.nvm/versions/node/v10.9.0/bin/npm
  Browsers:
    Firefox: 62.0
    Safari: 11.1.2
  npmPackages:
    gatsby: next => 2.0.0-rc.24
    gatsby-plugin-google-analytics: next => 2.0.0-rc.2
    gatsby-plugin-offline: next => 2.0.0-rc.7
  npmGlobalPackages:
    gatsby-dev-cli: 2.0.0-rc.5
    gatsby: 2.0.0-rc.24
@m-allanson
Copy link
Contributor Author

m-allanson commented Sep 14, 2018

Note that this will cause a Best Practices warning in Lighthouse audits.

In #7378, for next.gatsbyjs.org this error is triggered by using gatsby-plugin-glamor with the offline plugin and google analytics plugin:

screen shot 2018-09-14 at 11 54 59

@haroldangenent
Copy link
Contributor

@m-allanson / @davidbailey00: I still see the first error (in Chrome) when it tries to fetch an external resource (like Analytics).

Currently it does catch the TypeError, but (at least) Chrome also throws an error from the promise itself:

Failed to load https://www.google-analytics.com/analytics.js: No 'Access-Control-Allow-Origin' header is present on the requested resource.

Would it be a good approach to combine #8183 and #8158 to only try to fetch internal resources? Something like this:

resources.filter(isInternal).map(resource =>
  cache.add(resource)
)

Where isInternal would check if the URL is an internal URL. I'm not too familiar with the SW in gatsby-plugin-offline, but if you think this approach is valid I'm happy to prepare a PR.

@vtenfys
Copy link
Contributor

vtenfys commented Oct 30, 2018

@haroldangenent actually we don't do this because some URLs (e.g. web fonts) allow CORS, which means they can be cached. If we only cache internal resources then we might miss out on caching stuff like external fonts and CSS.

As you've stated, this does come with the downside of console errors for resources which don't allow CORS, but on the whole it's better to try to cache external resources rather than ignore all of them.

@haroldangenent
Copy link
Contributor

@davidbailey00 Thanks for your response. That makes sense. Maybe it would still be an idea to use the no-cors mode as done in #8158 (https://github.com/gatsbyjs/gatsby/pull/8158/files) for external resources? Also found this StackOverflow topic on the subject.

@vtenfys
Copy link
Contributor

vtenfys commented Oct 31, 2018

@haroldangenent If I remember correctly, using no-cors means the response is opaque, i.e. we can't see the response body, so we wouldn't be able to cache any external resources using that option unfortunately. I remember trying this before and it didn't work, so that's actually why I left it without using no-cors.

@haroldangenent
Copy link
Contributor

haroldangenent commented Nov 2, 2018

@davidbailey00 I see. I also did some tests out of curiosity (just in Chrome for now), and noticed that all internal responses (using mode: 'no-cors') returned a basic response-type. However, all external resources return an opaque response (even fonts).

Based on another StackOverflow answer, it seems that opaque responses should still be cacheable. In that case, something like this could work:

resources.map(resource =>
  fetch(resource, { mode: 'no-cors' })
    .then(response => cache.put(resource, response))
)

This would not only prevent the error from showing, but it would also cache the script (e.g. Google Analytics). Did some tests and it seems to be working as expected. As the answer states, there are some downsides:

  • An error response from an external resource might be cached inadvertedly (since there's no way of knowing the response header)
  • Opaque responses take up more space during quota calculations (padding is added to opaque responses)

If you think this approach would still be valid, let me know. I'll be happy to investigate some more and open up a PR for review. If not, we'll leave it as it is (and maybe document that these errors are expected).

@vtenfys
Copy link
Contributor

vtenfys commented Nov 3, 2018

@haroldangenent Thanks for looking into this, your findings are super helpful 👍

I'll create a PR today since this is a fairly simple change and I'm not working on tons of other things at the moment, but I'll let you know when once I've done that so you can have a look!

@vtenfys vtenfys reopened this Nov 3, 2018
DSchau pushed a commit that referenced this issue Nov 5, 2018
…use no-cors for external resources (#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 #8145
@haroldangenent
Copy link
Contributor

@davidbailey00 PR is looking good, thanks for your work on this!

gpetrioli pushed a commit to gpetrioli/gatsby that referenced this issue 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 a pull request may close this issue.

3 participants