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): add opt out flag SKIP_PLUGIN_OPTION_VALIDATION to bypass validation #27885

Closed
wants to merge 5 commits into from

Conversation

gillkyle
Copy link
Contributor

@gillkyle gillkyle commented Nov 6, 2020

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 and develop process errors out (process.exit(1)). To give an unblocking workaround for users frustrated by this behavior, this change allows the SKIP_PLUGIN_OPTION_VALIDATION to be set, a la:

SKIP_PLUGIN_OPTION_VALIDATION=true gatsby develop

To skip the validation and instead issue a warning that looks like this:

image

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

@gillkyle gillkyle requested a review from mxstbr November 6, 2020 21:36
@gillkyle gillkyle requested review from a team as code owners November 6, 2020 21:36
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Nov 6, 2020
@KyleAMathews
Copy link
Contributor

KyleAMathews commented Nov 7, 2020

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?

@KyleAMathews
Copy link
Contributor

Also encourage them to file bugs if they believe the validation is in error.

Copy link
Contributor

@mxstbr mxstbr left a 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!

packages/gatsby/src/bootstrap/load-plugins/validate.ts Outdated Show resolved Hide resolved
@wardpeet
Copy link
Contributor

wardpeet commented Nov 9, 2020

This is dangerous. By bypassing validation, default values will not be set and cause more troubles. We should allow unknown values instead.

@sidharthachatterjee sidharthachatterjee added topic: plugins Related to plugin system, themes & catch-all for plugins that don't have a label and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Nov 9, 2020
@mxstbr
Copy link
Contributor

mxstbr commented Nov 9, 2020

We should allow unknown values instead.

There could be other issues with the schemas. I think the better solution is to skip the process.exit if the flag is set (but still validate and set the default values), but still show the errors in the console?

if (errors > 0) {
process.exit(1)
}

gillkyle and others added 2 commits November 9, 2020 08:25
Co-authored-by: Matt Kane <matt@gatsbyjs.com>
Co-authored-by: Max Stoiber <contact@mxstbr.com>
@gillkyle
Copy link
Contributor Author

gillkyle commented Nov 9, 2020

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 hello-world site with one erroneous plugin installed:

CleanShot 2020-11-09 at 09 07 59@2x

Things I did:

  • switched process.exit to reporter.panic (a suggestion from Matt)
  • added the suggestion left in code review, encouraging users to file issues and tweaking error wording
  • moved the check of the flag that bypasses logic after the validation runs (based off of @wardpeet's comment) so that validation still runs
  • added another if around the reporter warning so errors are not logged if you have the flag set (it will only show the warnings and not the specific error message for each erroneous plugin)

Am I still missing things?

@gillkyle
Copy link
Contributor Author

gillkyle commented Nov 9, 2020

I'm going to look into the failing tests now, it looks like this might alter the assertions that some of them make..

@gillkyle gillkyle requested a review from mxstbr November 9, 2020 17:49
@gillkyle gillkyle added the status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response label Nov 9, 2020
Copy link
Contributor

@mxstbr mxstbr left a 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
Copy link
Contributor

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

Copy link
Contributor

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,`)
Copy link
Contributor

@mxstbr mxstbr Nov 10, 2020

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`)

@gillkyle
Copy link
Contributor Author

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.

@LekoArts
Copy link
Contributor

@gillkyle Can this be closed then?

@gillkyle
Copy link
Contributor Author

gillkyle commented Dec 1, 2020

Thanks for the reminder @LekoArts! It can indeed, Max shipped the warnings PR so this is irrelevant 🙂

@gillkyle gillkyle closed this Dec 1, 2020
@LekoArts LekoArts deleted the plugin-validation/add-flag-to-opt-out branch December 1, 2020 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response topic: plugins Related to plugin system, themes & catch-all for plugins that don't have a label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants