feat: start work on validating plugin options #16027

DSchau · 2019-07-23T22:18:40Z

Description

Extremely WIP work on implementing #15281

Issues to Solve

Implementing this API in a plugin is a breaking change (because Gatsby will fail with an unknown API error)
- Note: we should probably solve this problem, e.g. to implement new APIs isn't always a breaking change, e.g. a noop in older versions of Gatsby would've been OK here
I'd probably like to use this API to coerce values, as well (e.g. if expecting a number, Joi will coerce to a string a la this example) and set the result as pluginOptions for the plugin from that point
This should only be available to plugins (e.g. disallow in root-level gatsby-node.js?)
Need to set defaults on passed-in Joi instance
- allowUnknown: true to avoid empty plugins option causing issues
- abortEarly: false to show all errors

and probably more I'm forgetting. It's a start! 🚀

Still to do

Solve all those issues
Ensure not using the passed in validator doesn't cause any issues with the error message
Add tests
Dogfood with plugins in gatsby/* monorepo (e.g. gatsby-source-contentful)
Make structured error message a little nicer and actionable

Related Issues

pieh · 2019-07-24T10:37:28Z

3. This should only be available to plugins (e.g. disallow in root-level gatsby-node.js?)

Is this distinction that need to be made?

Only usecase I see where this could be problematic (depending how it's handled) is if there is theme that also doubles as starter (where you would want validation for theme options, but maybe not when using it as a starter (tho then, using default values might actually be good idea)

DSchau · 2019-07-24T13:45:39Z

Is this distinction that need to be made?

It feels like a "useless" API in the root of a project (because there aren't any options?), but yeah -- perhaps not a fix that needs to be made. In other words, if a user wants to use it (even though it's not valuable) -- so be it!

DSchau · 2019-07-25T14:19:40Z

Because this is quite breaking for any plugins that implement (without updating Gatsby in the consuming application), let's either:

Implement in plugins outside the scope of this PR, or
Wait until feat: start work on validating plugin options #16027 lands

I think I'm leaning towards 1 in the interest of shipping the base functionality, however, I do also think it's quite important to use the API in various plugins so that we can ensure it accommodates all use cases.

DSchau · 2019-08-12T16:14:25Z

This is no longer blocked since #16027 landed. I'll revisit work on this today and this week.

There will be some churn in any new APIs until people upgrade to the latest Gatsby, but I think that's OK.

pieh · 2019-08-12T17:34:25Z

This is no longer blocked since #16027 landed.

I think that's wrong PR/Issue number (it's links to this PR) ;)

DSchau · 2019-08-12T17:35:56Z

lol -- I double down'd on my assumption.

*** #16105

BREAKING: relying upon nested SitePlugin fields (pluginOptions and packageJson) is no longer supported. These are now typed as generic GraphQL `JSON` types to avoid any issues regarding missing or conflicting field types.

DSchau · 2019-08-13T00:31:54Z

OK -- got a lot done on this.

Values are coerced by Joi/validation
- Note: the idea is that validatePluginOptions returns either a Joi validation instance (e.g. Joi.object().keys()) or the user can directly return plugin options which get mutated.
Joi doesn't warn on e.g. plugins or other misc. fields; all errors are shown instead of just first
JSON types for SitePlugin fields
- Note: didn't do separate plugin options (e.g. SitePluginGatsbySourceContentful or anything) but I think the schema customization API solves that purpose rather nicely
Dogfooded on a plugin (gatsby-source-contentful)

What's not done?

Themes don't get their options coerced because their gatsby-config.js is a special option and we run this API after themes are loaded
- This will be a little challenging to solve but should be doable
- This is a "big deal" because we can set defaults in this API and coerce values, which we want to make available to gatsby-config.js and other plugins the theme may invoke
Makes errors as actionable as possible
- Joi's errors are only OK and don't always point to the best way to fix
Tests
Dogfood more packages

Example

DSchau · 2019-08-13T14:28:27Z

packages/gatsby/src/utils/api-runner-node.js

+           * validatePluginOptions is special
+           * it validates plugin options and (optionally) mutates them
+           */
+          if (api === `validatePluginOptions`) {


This feels extra janky. Any suggestions for improvements?

DSchau · 2019-08-13T14:29:19Z

packages/gatsby/src/utils/api-runner-node.js

+                  allowUnknown: true,
+                })
+              }
+              return resChain.then(options => {


Here's where options are actually coerced. A "gotcha" here is that whether using Joi or not, the returned structure is what will be passed to other APIs that consume plugin options.

DSchau · 2019-08-26T23:03:19Z

Note: I haven't forgotten about this (important!) PR -- but other priorities have come up. I'd like to circle back to this before the end of the week.

moonmeister · 2019-09-06T16:43:27Z

@DSchau because of the complex plugin options for gatsby-plugin-manifest it could be a good dog fooder. I should have some time in the next week to test this, anything i should know?

DSchau · 2019-09-06T16:45:00Z

@moonmeister I was just gonna pick this up again today!

Yeah -- that'd be a great one. It should be ready to test for plugins. Would you want to open a PR to this branch when you test things out?

moonmeister · 2019-09-06T16:50:39Z

@DSchau that works for me.

…option-validation

DSchau · 2019-09-19T14:13:20Z

Hey @moonmeister!

Awesome! Let me add a few comments/questions:

Having to add plugins: Joi.array() to every config's preferences is redundant and tedious (imo)

Why is this necessary? Removing "strict" (e.g. allowing unknown keys) should prevent this from being necessary? (which should be the behavior now, e.g. I don't add plugins to my Contentful test ->

gatsby/packages/gatsby-source-contentful/src/plugin-options.js

Lines 15 to 28 in 01f4b8f

    
           const getValidOptions = Joi => 
        
             Joi.object().keys({ 
        
               accessToken: Joi.string() 
        
                 .required() 
        
                 .empty(), 
        
               spaceId: Joi.string() 
        
                 .required() 
        
                 .empty(), 
        
               host: Joi.string().default(defaultOptions.host), 
        
               environment: Joi.string().default(defaultOptions.environment), 
        
               downloadLocal: Joi.boolean().default(defaultOptions.downloadLocal), 
        
               localeFilter: Joi.func().default(defaultOptions.localeFilter), 
        
               forceFullSync: Joi.boolean().default(defaultOptions.forceFullSync), 
        
             })

)

Agreed the concat approach would work, but ideally we don't need that at all.

moonmeister · 2019-09-19T14:30:10Z

@DSchau Hmm, I suppose I'd lean towards the strict mode by default. It eliminates the typo case. I meant to set property date and accidentally typed data (might have spent too much time debugging this very thing at work this week). Without strict mode I don't get any error that there is an unknown key and I have to notice it myself. If we could make this throw a warning that would be fine with me, but I think it's worth throwing something!

DSchau · 2019-09-19T14:32:02Z

@moonmeister I think you should check out the code I linked to!

Strict (as I used it) basically means it doesn't fail on extra keys passed that aren't part of the validation schema (in this case, plugins!).

In your example, if you set date as a known property, the schema itself would throw if you didn't pass date (e.g. if you passed data). I don't think that's impacted whatsoever in this flow with allowUnknown, and I also don't think we can prevent typos like that. That's sort of on the implementor of the validation 🤷‍♂

moonmeister · 2019-09-19T14:39:13Z

@DSchau Opps, you must have added code later, cause I didn't see it originally. Either way What you describe is what I'm suggesting, I want it to throw when I pass data cause it's not part of the validation schema. I only ran into this issue when i added .unknown(false) to my object.

DSchau · 2019-09-19T14:41:29Z

You could directly use Joi to workaround the baked-in defaults. I think what you're describing is a case that I'm not too concerned about.

If we think about your exact example, if you typo date as data in the validation schema, the validation will fail from the consumer of the plugin because they haven't passed date. Sure -- it won't fail if you merely only pass date (because it will be an unknown key) but I'm not sure I'm convinced of the negative impact, yet!

Happy to be proven wrong!

moonmeister · 2019-09-19T14:45:28Z

@DSchau Okay I get what your saying, there is still the case if data is optional then it won't error. I guess I don't see why making this strict and including plugins by default is such a big deal. Why is it "ideal" that we don't do this?

DSchau · 2019-09-19T14:47:26Z

@moonmeister what if a plugin wants to enforce a particular shape for plugins? We then have a non-overridable key, right?

I guess I'd toss it back to ya -- why is allowUnknown such a big deal? I'm not yet convinced of the negative impact!

Also to be clear, if this is a concern, you can use your own instance of Joi (or really any validation). Ours is passed as a convenience, it's not strictly necessary.

DSchau · 2019-09-19T15:02:11Z

@moonmeister in thinking more about my above statement -- I think plugins is a fairly typical known shape. It's either a string, or an object with keys resolve and options. I don't think this pattern falls down anywhere, so maybe it's OK to enforce it by default?

moonmeister · 2019-09-19T15:09:25Z

what if a plugin wants to enforce a particular shape for plugins? We then have a non-overidable key, right?

plugins would always be an optional array because gatsby always injects it (in the validation obj and in the plugin options). If a plugin wants to make it a required string they could. whatever is concatenated onto a Joi object merges with existing object properties but takes precedent. Meaning if required() is set on the plugins property in the object returned from the plugin then plugins becomes required. UPDATE: but as you've now noted if I were to override this to a random object that'd be breaking the Gatsby API. I think this should be in there to make sure it's being validated as Gatsby expects it.

I guess I'd toss it back to ya -- why is allowUnknown such a big deal? I'm not yet convinced of the negative impact!

It's not about the negative, it's about the positive experience it provides. I think I've articulated a very valid and probably common case where a dev (especially new ones) miss type or are trying to add properties not understanding where they should go. Explicitly setting allowUnknown to false would improve developer experience with no drawbacks that i can see. Right now I've forced my object to not allow but if another plugin really needed, they could force to allowUnknown. So why not?

DSchau · 2019-09-19T16:57:06Z

@moonmeister you've persuaded me! I'll keep thinking if there are any plugins that may not match this expected structure (I suppose we could just make it more generic, e.g. validate that plugins is an array!), but for now, I agree that this is an improvement.

It does mean that developers will have to be strict about valid options, but perhaps that's a valuable outcome?

See f0ef733 for the implementation and 870def4 for the tests

moonmeister · 2019-09-20T00:12:25Z

I think it's valuable.

…n options validation

…c they adhear to.

feat: add initial options validation

moonmeister · 2019-10-08T03:58:39Z

@DSchau Where we at on this? Anything else I can do to help?

LekoArts · 2020-10-22T09:17:05Z

This was successfully implemented in a bunch of others PRs :)
#27337

feat: start work on validating plugin options

57aab61

DSchau added status: blocked This issue/PR can't be solved at the moment and shouldn't be closed/merged status: WIP labels Jul 23, 2019

DSchau mentioned this pull request Jul 24, 2019

[feat] Improve experience of introducing new APIs to Gatsby Core #16055

Closed

DSchau mentioned this pull request Jul 26, 2019

feat(gatsby): enable richer error handling for unknown APIs #16105

Merged

3 tasks

DSchau removed the status: blocked This issue/PR can't be solved at the moment and shouldn't be closed/merged label Aug 12, 2019

DSchau added 3 commits August 12, 2019 14:13

feat: mutate plugin options and handle validatePluginOptions separately

bd60e63

chore(gatsby-source-contentful): rewrite to use new plugin validation

0943c61

DSchau commented Aug 13, 2019

View reviewed changes

DSchau added 5 commits September 6, 2019 12:25

feat: coerce theme options (jankily, I'm not really thrilled with this)

ae69a48

comment code because comments are good

6dd6154

tests are important mkay

1305b94

chore: get contentful unit tests passing

1c9f457

Merge branch 'master' of github.com:gatsbyjs/gatsby into feat/plugin-…

9f47605

…option-validation

DSchau added 2 commits September 19, 2019 09:36

chore: add plugins as known valid options and concat with instance

f0ef733

test: add tests

870def4

chore: move plugin schema to joi schemas

c097360

moonmeister and others added 6 commits September 20, 2019 09:19

feat: add initial options validation

42fe23c

feat: add validation for WebAppManifest spec and further refine plugi…

34a62d7

…n options validation

fix: refine manifest obj to better fit spec

c33fdb2

docs: add apendix indicating options are validated and version of spe…

e3e5de3

…c they adhear to.

tests: write tests, fix bugs, cleanup stuff

1e7a0e3

Merge pull request #13 from DSchau/feat/manifest-option-validation

deb44b5

feat: add initial options validation

DSchau removed the status: WIP label Sep 24, 2019

DSchau changed the title ~~[WIP] feat: start work on validating plugin options~~ feat: start work on validating plugin options Sep 24, 2019

m-allanson mentioned this pull request Dec 4, 2019

Warn on invalid config format #19941

Closed

LekoArts mentioned this pull request Jan 20, 2020

Unhelpful error messages for invalid gatsby-config properties #20704

Closed

danabrit removed effort: med labels May 28, 2020

LekoArts closed this Oct 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: start work on validating plugin options #16027

feat: start work on validating plugin options #16027

DSchau commented Jul 23, 2019 •

edited

Loading

pieh commented Jul 24, 2019

DSchau commented Jul 24, 2019 •

edited

Loading

DSchau commented Jul 25, 2019 •

edited

Loading

DSchau commented Aug 12, 2019

pieh commented Aug 12, 2019

DSchau commented Aug 12, 2019

DSchau commented Aug 13, 2019 •

edited by moonmeister

Loading

DSchau Aug 13, 2019

DSchau Aug 13, 2019

DSchau commented Aug 26, 2019

moonmeister commented Sep 6, 2019

DSchau commented Sep 6, 2019

moonmeister commented Sep 6, 2019

DSchau commented Sep 19, 2019 •

edited

Loading

moonmeister commented Sep 19, 2019 •

edited

Loading

DSchau commented Sep 19, 2019 •

edited

Loading

moonmeister commented Sep 19, 2019 •

edited

Loading

DSchau commented Sep 19, 2019 •

edited

Loading

moonmeister commented Sep 19, 2019

DSchau commented Sep 19, 2019

DSchau commented Sep 19, 2019

moonmeister commented Sep 19, 2019 •

edited

Loading

DSchau commented Sep 19, 2019

moonmeister commented Sep 20, 2019

moonmeister commented Oct 8, 2019

LekoArts commented Oct 22, 2020

feat: start work on validating plugin options #16027

feat: start work on validating plugin options #16027

Conversation

DSchau commented Jul 23, 2019 • edited Loading

Description

Issues to Solve

Still to do

Related Issues

pieh commented Jul 24, 2019

DSchau commented Jul 24, 2019 • edited Loading

DSchau commented Jul 25, 2019 • edited Loading

DSchau commented Aug 12, 2019

pieh commented Aug 12, 2019

DSchau commented Aug 12, 2019

DSchau commented Aug 13, 2019 • edited by moonmeister Loading

Example

DSchau Aug 13, 2019

Choose a reason for hiding this comment

DSchau Aug 13, 2019

Choose a reason for hiding this comment

DSchau commented Aug 26, 2019

moonmeister commented Sep 6, 2019

DSchau commented Sep 6, 2019

moonmeister commented Sep 6, 2019

DSchau commented Sep 19, 2019 • edited Loading

moonmeister commented Sep 19, 2019 • edited Loading

DSchau commented Sep 19, 2019 • edited Loading

moonmeister commented Sep 19, 2019 • edited Loading

DSchau commented Sep 19, 2019 • edited Loading

moonmeister commented Sep 19, 2019

DSchau commented Sep 19, 2019

DSchau commented Sep 19, 2019

moonmeister commented Sep 19, 2019 • edited Loading

DSchau commented Sep 19, 2019

moonmeister commented Sep 20, 2019

moonmeister commented Oct 8, 2019

LekoArts commented Oct 22, 2020

DSchau commented Jul 23, 2019 •

edited

Loading

DSchau commented Jul 24, 2019 •

edited

Loading

DSchau commented Jul 25, 2019 •

edited

Loading

DSchau commented Aug 13, 2019 •

edited by moonmeister

Loading

DSchau commented Sep 19, 2019 •

edited

Loading

moonmeister commented Sep 19, 2019 •

edited

Loading

DSchau commented Sep 19, 2019 •

edited

Loading

moonmeister commented Sep 19, 2019 •

edited

Loading

DSchau commented Sep 19, 2019 •

edited

Loading

moonmeister commented Sep 19, 2019 •

edited

Loading