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

fix(gatsby-plugin-utils): don't throw on warnings in pluginOptionsSchema #34182

Merged
merged 12 commits into from Jan 13, 2022

Conversation

moonmeister
Copy link
Contributor

@moonmeister moonmeister commented Dec 3, 2021

Description

Modifies error handling for plugin options schema validation to not throw on warnings like unknown keys. Before when throwing on warnings the updated(coerced or defaults added) options was not returned to subsequent API calls causing a bad bug.

Documentation

NA

Fixes #33033

[sc-44171]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Dec 3, 2021
@moonmeister
Copy link
Contributor Author

Looks like lots of tests are breaking now that defaults are correctly being passed.

@LekoArts LekoArts added topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Dec 8, 2021
@LekoArts LekoArts self-assigned this Dec 8, 2021
* master: (125 commits)
  chore(release): Publish next
  fix(gatsby): createNode return promise (gatsbyjs#34399)
  chore(release): Publish next
  fix(gatsby): Wrong route resolved by findPageByPath function (gatsbyjs#34070)
  fix(deps): update typescript to v5 (major) (gatsbyjs#33786)
  chore(docs): Update processing external images guide (gatsbyjs#34388)
  chore(deps): update dependency aws-sdk to ^2.1048.0 (gatsbyjs#34365)
  chore(deps): update dependency autoprefixer to ^10.4.1 for gatsby-plugin-sass (gatsbyjs#34357)
  chore(deps): update formatting & linting (gatsbyjs#34370)
  fix(deps): update minor and patch dependencies for gatsby-source-drupal (gatsbyjs#34375)
  fix(deps): update dependency eslint-plugin-react to ^7.28.0 (gatsbyjs#34372)
  fix(deps): update dependency resolve-url-loader to ^3.1.4 for gatsby-plugin-sass (gatsbyjs#34361)
  chore(deps): update dependency typescript to ^4.5.4 (gatsbyjs#34358)
  chore(docs): Fix links to shared layout component (gatsbyjs#34330)
  chore: Fix typo (gatsbyjs#34349)
  chore(examples): use mobx v6 in using-mobx example (gatsbyjs#34351)
  chore(deps): update dependency rewire to v6 for gatsby-plugin-offline (gatsbyjs#34376)
  chore(deps): update dependency msw to ^0.36.3 for gatsby-core-utils (gatsbyjs#34367)
  chore(deps): update dependency msw to ^0.36.3 for gatsby-plugin-gatsby-cloud (gatsbyjs#34368)
  fix(deps): update dependency graphql to ^15.8.0 for gatsby-codemods (gatsbyjs#34373)
  ...
@moonmeister

This comment has been minimized.

@moonmeister
Copy link
Contributor Author

@LekoArts All remaining failing tests are unrelated to my changes.

@tyhopp tyhopp self-assigned this Jan 10, 2022
@tyhopp tyhopp mentioned this pull request Jan 11, 2022
* master: (24 commits)
  chore(docs): Release Notes v4.5 (gatsbyjs#34425)
  chore(docs): Update quick-start guide (gatsbyjs#34445)
  chore(docs) : Typo fix GatbsyImage -> GatsbyImage (gatsbyjs#34439)
  perf(gatsby): reuse rootNode & trackedRootNodes caches across instances of graphqlRunner (gatsbyjs#33695)
  Update media-item-processing.md (gatsbyjs#34434)
  chore(docs): Update localization doc (gatsbyjs#34429)
  test(ssr): Fix flakes (gatsbyjs#34443)
  chore(release): Publish next
  Revert "docs: Match egghead.io video instructions (gatsbyjs#34315)" (gatsbyjs#34384)
  fix(gatsby-plugin-manifest): generate icons sequentially (gatsbyjs#34331)
  Fix misspelling of "precedence" in log message (gatsbyjs#34428)
  chore(docs): Adjust doc mentions of gatsby-plugin-create-client-paths (gatsbyjs#34424)
  chore(docs): Update static-folder doc (gatsbyjs#34392)
  Upgrade to strip-ansi ^6.0.1 (gatsbyjs#34383)
  chore(gatsby-plugin-create-client-paths): Update client paths plugin readme with migration info (gatsbyjs#34423)
  chore: Remove deprecated client paths plugin references (gatsbyjs#34422)
  chore(docs): Old occurrences of gatbyjs.org (gatsbyjs#34402)
  Update plugins.md to have correct URL for gatsby-plugin-segment-js (gatsbyjs#34397)
  chore(gatsby): Give option to ignore output from workers and silence validate-engines (gatsbyjs#34416)
  chore(release): Publish next pre-minor
  ...
Copy link
Contributor

@tyhopp tyhopp left a comment

Choose a reason for hiding this comment

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

Hi @moonmeister, thanks for putting this up! I added a few comments we can discuss, otherwise it's looking good and works as intended.

@tyhopp tyhopp added the status: awaiting author response Additional information has been requested from the author label Jan 12, 2022
@tyhopp
Copy link
Contributor

tyhopp commented Jan 12, 2022

@moonmeister I had a look at the last failing test and merged #34443 that should (hopefully) get these all passing if you rebase on master.

* master:
  fix(gatsby-core-utils): Re-Export updateSiteMetadata (gatsbyjs#34462)
  fix(deps): update starters and examples gatsby packages to ^4.5.1 (gatsbyjs#34463)
  fix(gatsby-dev-cli): Do not ignore engines (gatsbyjs#34461)
  fix(gatsby): Revert gatsbyjs#33786 (gatsbyjs#34458)
  fix(deps): update starters and examples - gatsby (gatsbyjs#34449)
  chore(changelogs): update changelogs (gatsbyjs#34451)
  Update tutorial part 1 to reflect changes to add site flow (gatsbyjs#34427)
  preview 2.0 UI changes (gatsbyjs#34440)
@moonmeister
Copy link
Contributor Author

@tyhopp SSR test is still failing.

@tyhopp
Copy link
Contributor

tyhopp commented Jan 13, 2022

@tyhopp SSR test is still failing.

It does seem so, although only one assertion fails now instead of all like before. I'll have a look today around the codebase to see what patterns we use for waiting for non-arbitrary amounts of time (e.g. CLI output or a specific network request).

@moonmeister
Copy link
Contributor Author

LGTM!

@tyhopp tyhopp merged commit 252f50d into gatsbyjs:master Jan 13, 2022
@LekoArts LekoArts changed the title fix(gatsby): don't throw on warnings in pluginOptionsSchema fix(gatsby-plugin-utils): don't throw on warnings in pluginOptionsSchema Jan 13, 2022
@tyhopp
Copy link
Contributor

tyhopp commented Jan 13, 2022

@moonmeister Just want to share that this is released in gatsby-plugin-utils@3.0.0-next.0 since it includes a breaking change.

pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
…js#34182)

* fix: don't throw on warnings

* fix: typings

* fix: more type fixes

* fix: tests and add correct test

* tests: fix tests based on cahnges

* fix: resolve final test issues

* cleanup: remove test console.log

* add https to github link

* test: fix incorect snapshot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting author response Additional information has been requested from the author topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(node-api: pluginOptionsSchema): fails to pass validate options on warning
3 participants