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): delay adding resources for paths until we have urls #8613

Merged
merged 15 commits into from
Oct 9, 2018
Merged

fix(gatsby-plugin-offline): delay adding resources for paths until we have urls #8613

merged 15 commits into from
Oct 9, 2018

Conversation

vtenfys
Copy link
Contributor

@vtenfys vtenfys commented Sep 28, 2018

This adds a new API called onPostPrefetchPathname, which runs after resources have been fetched for a pathname, as opposed to onPrefetchPathname which runs directly upon fetching resources.

In plugin-offline, the existing API onPrefetchPathname was being used - this was causing problems, since the resources weren't always fetched in time for getResourceURLsForPathname to return the data (see https://github.com/gatsbyjs/gatsby/pull/8613/files#diff-450f89b4ad11867864b3d8d21f83e442R35 - getResourceURLsForPathname sometimes returned null).

Now that the API onPostPrefetchPathname is used instead, getResourceURLsForPathname will always work properly, since the API runs after the resources are fetched and only if they are fetched successfully.

In addition to this, I've passed onPostPrefetchPathname into the API runner for onPrefetchPathname so that plugins which use custom prefetching can run this API at the right time.

@vtenfys vtenfys requested a review from a team as a code owner September 28, 2018 11:16
@vtenfys
Copy link
Contributor Author

vtenfys commented Sep 28, 2018

AppVeyor why...

image

@KyleAMathews
Copy link
Contributor

This is called when the "prefetching" has started not when the resources has actually been downloaded.

Can you explain more about the error plugin-offline is causing?

@vtenfys
Copy link
Contributor Author

vtenfys commented Sep 28, 2018

@KyleAMathews We need to guarantee that getResourceURLsForPathname will actually work e.g. in gatsby-plugin-offline (see link in updated description) - currently it's sometimes returning null, since the URLs have not been populated in time, or it returns null if the pathname is a 404 (e.g. integration-tests/production-runtime for "/page-3").

With the new API, it's called at a time when getResourceURLsForPathname is guaranteed to work, so there's no longer an error like "Cannot call forEach of null` in plugin-offline (again at the link in the updated description).

@vtenfys vtenfys changed the title [v2] [plugin-offline] [API] Fix fetching resources before prefetch finishes [v2] [plugin-offline] [API] Fix fetching resources before resource URL prefetch finishes Sep 28, 2018
@@ -69,6 +73,8 @@ exports.onPrefetchPathname = ({ pathPrefix }, pluginOptions) => {
resources.push(`static/d/${___dataPaths[page.jsonName]}.json`)
// TODO add support for pathPrefix
resources.forEach(r => prefetch(`/${r}`))

onPostPrefetchPathname({ pathname: p })
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we refactor this a bit so loader.js exposes prefetchResourcesForPath function and guess plugin and default gatsby prefetching use same implementation (and onPostPrefetchPathname api is called just from loader.js) ?

@pieh pieh changed the title [v2] [plugin-offline] [API] Fix fetching resources before resource URL prefetch finishes fix(gatsby-plugin-offline): delay adding resources for paths until we have urls Oct 9, 2018
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Thanks @davidbailey00!

@pieh pieh merged commit 2605aa0 into gatsbyjs:master Oct 9, 2018
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