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-source-contentful): Improve network error handling #30257

Merged
merged 18 commits into from
Mar 29, 2021

Conversation

axe312ger
Copy link
Collaborator

@axe312ger axe312ger commented Mar 15, 2021

  • improve sync progress & error logging
  • reintroduce retry logic from Contentful SDK
  • allow setting of any value of Contentful SDK again (VERY helpful for tests and debugging aka setting request headers)
  • add tests

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Mar 15, 2021
@axe312ger axe312ger added status: needs core review Currently awaiting review from Core team member topic: source-contentful Related to Gatsby's integration with Contentful and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Mar 15, 2021
@axe312ger axe312ger marked this pull request as ready for review March 15, 2021 16:33
@axe312ger axe312ger marked this pull request as draft March 15, 2021 18:39
@axe312ger axe312ger force-pushed the fix/contentful-improve-network-error-handling branch from 495f495 to 8946b74 Compare March 16, 2021 10:22
@wardpeet
Copy link
Contributor

Can you elaborate on "allow setting of any value of Contentful SDK again (VERY helpful for tests and debugging aka setting request headers)". Why would you want to augment it, instead of adding these values to the plugin itself?

Why don't we support a retry option, ...? You'll also need to update plugin validation in gatsby-node

@axe312ger
Copy link
Collaborator Author

axe312ger commented Mar 16, 2021

@wardpeet: the Contentful SDK allows a lot of options, some might be relevant in rare use cases. Most people won't need this at all. Keeping these in sync with Gatsby configuration is extra work we can avoid. https://github.com/contentful/contentful.js/#configuration

This PR needs it to speed up the tests, otherwise it would retry 30seconds in one test.

Also:

  • this feature existed before but got broken
  • I need this to be backported, adding single values is adding new features and we do not add features to the v2 branch
  • Adding them as options directly to the source plugin will encourage people to play with the values. I don't want them to do that, only people that have experience with the Contentful SDK should set them
  • the retryLimit is not even documented in the SDK docs :D - the SDK just passes anything down to axios :D is pretty much hidden, the README of the SDK is not up2date. https://contentful.github.io/contentful.js/contentful/8.2.0/contentful.html#.createClient

@axe312ger axe312ger force-pushed the fix/contentful-improve-network-error-handling branch from db291ba to 6da14b6 Compare March 16, 2021 12:20
@axe312ger axe312ger marked this pull request as ready for review March 16, 2021 13:41
@@ -181,6 +179,12 @@ Number of entries to retrieve from Contentful at a time. Due to some technical l

Number of workers to use when downloading Contentful assets. Due to technical limitations, opening too many concurrent requests can cause stalled downloads. If you encounter this issue you can set this param to a lower number than 50, e.g 25.

**`contentfulClientConfig`** [object][optional] [default: `{}`]

Additional config which will get passed to [Contentfuls JS SDK](https://github.com/contentful/contentful.js#configuration).
Copy link

Choose a reason for hiding this comment

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

Should we add retryLimit in the SDK readme?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, but if you do, pls make sure to add the other potential missing config options there as well :)

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.

I've added some minor comments, I'm not 100% confident on the pluginOptions part as we don't have tests for it. We can test when we merge this and try out the @next package.

const createContentfulErrorMessage = e => {
// If we got a response, it is very likely that it is a Contentful API error.
if (e.response) {
let parsedContentfulErrorData = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

we shouldn't set empty object here, null is fine

Suggested change
let parsedContentfulErrorData = {}
let parsedContentfulErrorData = null

Comment on lines 26 to 28
if (typeof parsedContentfulErrorData !== `object`) {
throw new Error()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

JSON.parse('string') will throw an error when it's not parsable. Unsure what you're trying to guard against.


let errorMessage = [
// Generic error values
e.code && `${e.code}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
e.code && `${e.code}`,
e.code && String(e.code),

packages/gatsby-source-contentful/src/fetch.js Outdated Show resolved Hide resolved
packages/gatsby-source-contentful/src/fetch.js Outdated Show resolved Hide resolved
@axe312ger
Copy link
Collaborator Author

@wardpeet I (re)implemented pluginOptions that way because I try to avoid adding new features to this PR and just restore old ones.

I need this to be merged into v4 of gatsby-source-contentful and that version has feature freeze due to Gatsby v3 release (has it?)

Alternatively I could directly implement maxRetries and headers and do not allow passing "anything" to Contentful SDK. Would you prefer this?

@axe312ger
Copy link
Collaborator Author

Alternative #2:

Remove the pluginOptions completely and let the retry test run for 30+ seconds. The real real reason I re-added it was to make this test take less time: https://github.com/gatsbyjs/gatsby/pull/30257/files#diff-ff025519f6b1f94c06ec41b72834e0426b77069be638c92bbc3eb34a83d1573dR80

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.

LGTM! THank you!

@axe312ger axe312ger added bot: merge on green Gatsbot will merge these PRs automatically when all tests passes and removed status: needs core review Currently awaiting review from Core team member labels Mar 23, 2021
@wardpeet wardpeet changed the title Contentful: Improve network error handling fix(gatsby-source-contentful): Improve network error handling Mar 29, 2021
@wardpeet wardpeet merged commit c1ac5e4 into master Mar 29, 2021
@wardpeet wardpeet deleted the fix/contentful-improve-network-error-handling branch March 29, 2021 08:38
vladar pushed a commit that referenced this pull request Mar 30, 2021
Co-authored-by: Ward Peeters <ward@coding-tech.com>
(cherry picked from commit c1ac5e4)
vladar pushed a commit that referenced this pull request Mar 30, 2021
#30568)

Co-authored-by: Ward Peeters <ward@coding-tech.com>
(cherry picked from commit c1ac5e4)

Co-authored-by: Benedikt Rötsch <axe312ger@users.noreply.github.com>
pieh pushed a commit that referenced this pull request Apr 1, 2021
Co-authored-by: Ward Peeters <ward@coding-tech.com>
(cherry picked from commit c1ac5e4)
pieh pushed a commit that referenced this pull request Apr 1, 2021
Co-authored-by: Ward Peeters <ward@coding-tech.com>
(cherry picked from commit c1ac5e4)
pieh pushed a commit that referenced this pull request Apr 2, 2021
#30617)

Co-authored-by: Ward Peeters <ward@coding-tech.com>
(cherry picked from commit c1ac5e4)

Co-authored-by: Benedikt Rötsch <axe312ger@users.noreply.github.com>
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 topic: source-contentful Related to Gatsby's integration with Contentful
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants