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

chore(gatsby-source-contentful): move to pluginOptionsSchema #27322

Merged
merged 7 commits into from
Oct 14, 2020

Conversation

mxstbr
Copy link
Contributor

@mxstbr mxstbr commented Oct 7, 2020

Based on the pluginOptionsSchema support added in #27242 this adds a pluginOptionsSchema to gatsby-source-contentful based on the already-existing Joi schema and custom option validation this plugin has.

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Oct 7, 2020
packages/gatsby-source-contentful/src/gatsby-node.js Outdated Show resolved Hide resolved
throw new Error(`Fetch call to check Contentful access failed.`)
})

return undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is because there's an early return, but since this isn't a .ts file, I'm not sure?

Copy link
Contributor

Choose a reason for hiding this comment

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

For external fro Joi API doc https://github.com/sideway/joi/blob/master/API.md#anyexternalmethod-description

which can either return a replacement value, undefined to indicate no change, or throw an error.

@pvdz pvdz added 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 Oct 7, 2020
@abhiaiyer91 abhiaiyer91 requested a review from wardpeet October 8, 2020 15:10
@abhiaiyer91
Copy link
Contributor

This looks good to me! whats the deal with test suite errors tho?

@mxstbr
Copy link
Contributor Author

mxstbr commented Oct 8, 2020

I haven't deleted any of the existing tests for the previous custom validation step yet. 😬

@mxstbr
Copy link
Contributor Author

mxstbr commented Oct 9, 2020

I cleaned up the error message to make it clearer, this should be ready to ship as a first MVP! 👍

@mfrachet
Copy link
Contributor

Now that we can rely on https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-plugin-utils/src/test-plugin-options-schema.ts maybe it can be a good idea to write tests and provide examples of the way to use it in a concrete case?

@abhiaiyer91
Copy link
Contributor

Do we need to wrap exports with the experimental flag?

@mxstbr
Copy link
Contributor Author

mxstbr commented Oct 12, 2020

maybe it can be a good idea to write tests and provide examples of the way to use it in a concrete case?

Yes for sure! 👍

Do we need to wrap exports with the experimental flag?

Yes we should unfortunately because the "does Node API exist" check throws an error in the current version of gatsby. See #27370.

@wardpeet wardpeet force-pushed the gatsby-source-contentful/plugin-options-schema branch from 73b979a to 7ef977c Compare October 14, 2020 15:59
@wardpeet wardpeet added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Oct 14, 2020
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.

Nice work! Looks good to me!

@gatsbybot gatsbybot merged commit ae9f9a9 into master Oct 14, 2020
@delete-merged-branch delete-merged-branch bot deleted the gatsby-source-contentful/plugin-options-schema branch October 14, 2020 17:05
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.

8 participants