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

feat(gatsby-plugin-utils): save validation resulting value to plugin options #27381

Merged
merged 10 commits into from
Oct 19, 2020

Conversation

moonmeister
Copy link
Contributor

Description

Some cleanup but more importantly saving results of Joi validation back to pluginOptions.

Documentation

I think @mxstbr is working on this but happy to add anything that's needed.

…nd us extends to extend Joi types, feat: save validation results back into plugin options.
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Oct 11, 2020
Copy link
Contributor Author

@moonmeister moonmeister left a comment

Choose a reason for hiding this comment

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

Some comments on the code and questions.

packages/gatsby-plugin-utils/src/joi.ts Outdated Show resolved Hide resolved
packages/gatsby-plugin-utils/src/validate.ts Outdated Show resolved Hide resolved
// All plugins have "plugins: []"" added to their options in load.ts, even if they
// do not have subplugins. We add plugins to the schema if it does not exist already
// to make sure they pass validation.
if (typeof plugin.pluginOptions === `undefined`) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for some reason pluginOptions can be undefined, so I've handled that case (thanks ts).

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have a repro cause it shouldn't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not specifically. But type script believed it could be. I'm guessing TS is wrong but I didn't try and hunt down where that was coming from. I can try and figure that out and let you know.

Copy link
Contributor

Choose a reason for hiding this comment

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

that means our gatsby types are wrong I guess ^^

Copy link
Contributor

Choose a reason for hiding this comment

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

@mxstbr this can be done in a follow-up PR but I would like to see where this comes from cause it means something in our types are wrong

Copy link
Contributor

Choose a reason for hiding this comment

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

/** Options passed to the plugin */
pluginOptions?: IPluginInfoOptions
}

Comment on lines 34 to 36
"peerDependencies": {
"gatsby": "^2.24.73"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

sweet, this is better 👍

@LekoArts LekoArts added topic: internal topic: plugins Related to plugin system, themes & catch-all for plugins that don't have a label type: feature or enhancement Issue that is not a bug and requests the addition of a new feature or enhancement. and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Oct 12, 2020
@mxstbr
Copy link
Contributor

mxstbr commented Oct 14, 2020

Note: after shipping this we're going to have to bump the minimum supported gatsby version for pluginOptionsSchema. Reference: #27437

@mxstbr mxstbr requested a review from a team October 16, 2020 11:07
@mxstbr
Copy link
Contributor

mxstbr commented Oct 16, 2020

Note: I had to re-add gatsby as a devDependency in 52118ee as we rely on importing some types from it, which otherwise didn't exist when only specified as a peerDep as far as I can tell!

@moonmeister
Copy link
Contributor Author

@mxstbr I've lost track on what needs to be done. Are you waiting on me for anything?

@mxstbr
Copy link
Contributor

mxstbr commented Oct 19, 2020

@moonmeister no, I went ahead and cleaned everything up! This just needs another code review 😊

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.

Looks good, One small follow up request.

// All plugins have "plugins: []"" added to their options in load.ts, even if they
// do not have subplugins. We add plugins to the schema if it does not exist already
// to make sure they pass validation.
if (typeof plugin.pluginOptions === `undefined`) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@mxstbr this can be done in a follow-up PR but I would like to see where this comes from cause it means something in our types are wrong

@mxstbr mxstbr merged commit 0073fb1 into master Oct 19, 2020
@mxstbr mxstbr deleted the moonmeister/feat/save-validated-plugin-options branch October 19, 2020 12:17
@@ -26,6 +26,8 @@ export {
withAssetPrefix,
} from "gatsby-link"

export { IPluginInfoOptions } from "./src/bootstrap/load-plugins/types"

Choose a reason for hiding this comment

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

This line broke TypeScript types as the src folder is not included in the distribution, so you get...

node_modules/gatsby/index.d.ts:29:36 - error TS2307: Cannot find module './src/bootstrap/load-plugins/types' or its corresponding type declarations.

29 export { IPluginInfoOptions } from "./src/bootstrap/load-plugins/types"

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you open a new issue for this please? 🙏

Choose a reason for hiding this comment

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

pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
…options (gatsbyjs#27381)

* misc: fix various validation issues in plugin-utils, uncopy typings and us extends to extend Joi types, feat: save validation results back into plugin options.

* fix: add missing comment

* fix: add missing comment

* fix: move gatsby to peer dep

* Undo Joi change

* Add test for this

* Update tests that are failing with new functionality

* Add gatsby as a devDep to gatsby-plugin-utils as we need the types

* Maybe fix type import

Co-authored-by: Max Stoiber <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: plugins Related to plugin system, themes & catch-all for plugins that don't have a label type: feature or enhancement Issue that is not a bug and requests the addition of a new feature or enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants