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): add timeout so fetching API info doesn't fail on very slow connections #17735

Merged
merged 5 commits into from
Sep 20, 2019

Conversation

KyleAMathews
Copy link
Contributor

@KyleAMathews KyleAMathews commented Sep 19, 2019

I was on an airplane trying to run gatsby develop and bootstrap hung here. I dug around and found adding a timeout fixed it.

@KyleAMathews KyleAMathews requested a review from a team as a code owner September 19, 2019 03:08
@muescha
Copy link
Contributor

muescha commented Sep 19, 2019

M
I am right that the latestApis only used in:

const getErrorContext = (badExports, exportType, currentAPIs, latestAPIs) => {
const entries = badExports.map(ex => {
return {
...ex,
api: latestAPIs[exportType][ex.exportName],
}
})

And only in case of error:

const context = getErrorContext(
entries,
exportType,
currentAPIs,
latestAPIs
)
reporter.error({
id: `11329`,
context,
})

Errors are not so much here.

That's why I ask:

  1. why we need to fetch on every gatsby develop the lastestApis from an external source?
  2. would it be enough only in case of error to fetch this data?
    3) why this latestapis need to stay on an external server? --> [feat] Improve experience of introducing new APIs to Gatsby Core #16055

@KyleAMathews
Copy link
Contributor Author

would it be enough only in case of error to fetch this data?

Good question. @DSchau didn't you add this originally? Thoughts?

@DSchau
Copy link
Contributor

DSchau commented Sep 19, 2019

@KyleAMathews yep -> #16105 (comment)

Wouldn't reasonably slow connections short-circuit here and surpass the 1s limit? Perhaps that's OK. (i.e. faster than airplane, slower than "good")

@muescha I like your idea to only fetch these APIs on error. That's a nicer solution.

@KyleAMathews I'll PR some tweaks and get this merged!

@KyleAMathews KyleAMathews requested a review from a team as a code owner September 19, 2019 23:27
@DSchau
Copy link
Contributor

DSchau commented Sep 19, 2019

@KyleAMathews I tried a new approach in 4f812d7.

This will only make the API request when bad exports are detected, which seems like a better approach. I added a few tests, and validated locally.

Seems fine and would solve the underlying issue you identified as the API request will no longer be made in bootstrap unless there are bad exports.

@muescha
Copy link
Contributor

muescha commented Sep 19, 2019

in case of timeout:

  • it would be not good to have just an fetch timeout error
  • it would be good to print then an describing error message about the signature check and the user should byself lookup at the file later - so he can go on and debug and can see easy errors like small typos

@muescha
Copy link
Contributor

muescha commented Sep 20, 2019

Maybe add also some meta data to the apis.json So we can print also infos like:

You are running now gatsby version 2.0 there is a newer version 2.1 available.

@KyleAMathews
Copy link
Contributor Author

Looks great! Yeah maybe have a timeout of e.g. five seconds. You could still fetch the data on bootstrap but just don't make the call blocking.

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Added a small comment about unused arg. This looks good, by default axios does not timeout so probably best to add a timeout option like @KyleAMathews mentioned.

packages/gatsby/src/bootstrap/load-plugins/index.js Outdated Show resolved Hide resolved
DSchau and others added 2 commits September 20, 2019 08:39
@DSchau
Copy link
Contributor

DSchau commented Sep 20, 2019

@muescha

it would be not good to have just an fetch timeout error

It falls through to the catch block with ECONABORTED, which will grab the (hopefully cached) or current apis.json.

Maybe add also some meta data to the apis.json So we can print also infos like:

You are running now gatsby version 2.0 there is a newer version 2.1 available.

This is what it does. If you're using an API in a later version of Gatsby (but with an older version) you'll get a prompt as to what version it was introduced in.

I've added a timeout (5s seems fair), this should be good to go.

@DSchau DSchau changed the title fix(core): Add timeout so fetching API info doesn't fail on very slow connections (e.g. airplanes) fix(gatsby): Add timeout so fetching API info doesn't fail on very slow connections (e.g. airplanes) Sep 20, 2019
@DSchau DSchau changed the title fix(gatsby): Add timeout so fetching API info doesn't fail on very slow connections (e.g. airplanes) fix(gatsby): add timeout so fetching API info doesn't fail on very slow connections Sep 20, 2019
@DSchau DSchau added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Sep 20, 2019
@DSchau
Copy link
Contributor

DSchau commented Sep 20, 2019

Going to merge and publish -- thanks Kyle for tagging me in!

@DSchau DSchau merged commit e85278c into master Sep 20, 2019
@DSchau DSchau deleted the timeout branch September 20, 2019 21:44
@DSchau
Copy link
Contributor

DSchau commented Sep 20, 2019

Successfully published gatsby@2.15.20

LOLdevelopr pushed a commit to LOLdevelopr/gatsby-1 that referenced this pull request Sep 21, 2019
…ow connections (gatsbyjs#17735)

* Add timeout so fetching API info doesn't fail on very slow connections (e.g. airplanes)

* fix(gatsby): only make a network request for latest APIs on error

* Update packages/gatsby/src/bootstrap/load-plugins/index.js

Co-Authored-By: Ward Peeters <ward@coding-tech.com>

* chore: Add a timeout

* chore: fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants