-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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): add opt out flag SKIP_PLUGIN_OPTION_VALIDATION to bypass validation #27885
Conversation
How about when we error for plugin validation we also tell people about this flag in the terminal so they can quickly get around bugs while it's getting fixed? |
Also encourage them to file bugs if they believe the validation is in error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like what @kylemathews suggested above!
This is dangerous. By bypassing validation, default values will not be set and cause more troubles. We should allow unknown values instead. |
There could be other issues with the schemas. I think the better solution is to skip the gatsby/packages/gatsby/src/bootstrap/load-plugins/validate.ts Lines 282 to 284 in 557139e
|
Co-authored-by: Matt Kane <matt@gatsbyjs.com> Co-authored-by: Max Stoiber <contact@mxstbr.com>
Okay I tried to make updates based on everyone's feedback and I think this would address them for now. Here's what happens running with and without the flag in a Things I did:
Am I still missing things? |
I'm going to look into the failing tests now, it looks like this might alter the assertions that some of them make.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fantastic to me! Love the behaviour as well 💯 I've added some bikeshedding nitpicks inline, feel free to postpone or implement them now — up to you.
If you are running your site and run into an error trying to develop or build your site due to plugin options failing that are undocumented or seem erroneous, you can add the `SKIP_PLUGIN_OPTION_VALIDATION` flag set to `true`. It can be prepended to your develop and build scripts, included as an [environment variable](/docs/environment-variables/) in a .env file, or included before your `gatsby develop` command like this: | ||
|
||
```shell | ||
SKIP_PLUGIN_OPTION_VALIDATION=true gatsby develop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bikeshedding, feel free to ignore/postpone
I'd rather not add this to the docs so explicitly because we don't really want people to use this unless they have a blocking error.
I would prefer if we add that information to the error log if they have an error with an ominous warning, for example something like:
success open and validate gatsby-configs - 0.017s
ERROR #11331 PLUGIN
Invalid plugin options for "gatsby-plugin-manifest":
- "name" must be a string
If you are certain your configuration is correct, you can disable configuration validation with the SKIP_PLUGIN_OPTION_VALIDATION environment variable. Be careful, this could have unintended side effects!
not finished load plugins - 2.391s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, this is a fun one. Ya, I totally see what you mean. At the same time, how often do people skip error messages and go googling? No strong feelings, just a weird thought exercise.
@@ -280,7 +282,13 @@ export async function validateConfigPluginsOptions( | |||
config.plugins = plugins | |||
|
|||
if (errors > 0) { | |||
process.exit(1) | |||
if (process.env.SKIP_PLUGIN_OPTION_VALIDATION) { | |||
reporter.warn(`you have enabled the SKIP_PLUGIN_OPTION_VALIDATION flag,`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: I'd rather we mention explicitly everytime that there is incorrect configuration, something like:
reporter.warn(`your gatsby-config.js contains invalid plugin options`)
reporter.warn(`because you have enabled the SKIP_PLUGIN_OPTION_VALIDATION flag, develop will try continuing`)
reporter.warn(`but this could have unintended side effects and break your site`)
With #27938 merged, which in my opinion is the ideal solution to this problem, I'm going to pause on this for now. I suspect we may not need to add this flag at all since warnings will surface when a plugin has an unknown option which is the majority of problems that have been encountered so far. Max noted it's possible there could be some errors not caught by warnings but I'm not sure we've seen too many of those to justify needing the flag if that PR covers most issues. I'm all ears if someone else feels more strongly but I'm going to wait a little before closing this, just to see if anyone has more justification for it. |
@gillkyle Can this be closed then? |
Thanks for the reminder @LekoArts! It can indeed, Max shipped the warnings PR so this is irrelevant 🙂 |
Description
Some users of the latest versions of Gatsby have run into problems with undocumented plugin options we didn't catch when adding schemas. When schema validation fails, the
build
anddevelop
process errors out (process.exit(1)
). To give an unblocking workaround for users frustrated by this behavior, this change allows theSKIP_PLUGIN_OPTION_VALIDATION
to be set, a la:To skip the validation and instead issue a warning that looks like this:
An alternative approach to implementing this would be warning that options not included in the schema have separate warnings issued for them, rather than erroring. It looks like this should be possible with Joi (see the
warnings
option) but I couldn't get it to work and figured this would be a relatively quick way to get around this without digging into Joi and have a similar effect.Documentation
I added a note to the plugin options guide
Related Issues
Related to #27839