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): log config validation errors #23620

Merged
merged 4 commits into from
May 4, 2020

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Apr 30, 2020

Description

This fixes error that hides actual gatsby-config validation error.

It also converts config reducer to TS, adds typed action creator and converts validation error to structured one.

Before (yarn build - this one was hiding actual error):
Screenshot 2020-04-30 at 02 28 31

Before (gatsby build - this was "fine" - bit weirdly formatted - for the most part this PR just changes it to structured error in this case)
Screenshot 2020-04-30 at 02 29 12

After (both yarn build and gatsby build):
Screenshot 2020-04-30 at 02 26 34

Related Issues

Fixes: #22812
Helps with #23468 (doesn't fix it, but makes it easier to see actual error)

@pieh pieh requested a review from a team as a code owner April 30, 2020 00:30
Comment on lines +35 to +37
linkPrefix: Joi.forbidden().error(
new Error(`"linkPrefix" should be changed to "pathPrefix"`)
),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved from

if (normalizedPayload.linkPrefix) {
console.log(`"linkPrefix" should be changed to "pathPrefix"`)
}
to validation object, so validation is unified

const action = setSiteConfig({})
expect(action.payload.pathPrefix).toBe(``)
})
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are tests that previously were reducer tests adjusted to be action creator test (same scenarios)

@@ -47,6 +47,9 @@ export interface IGatsbyConfig {
title?: string
author?: string
description?: string
sireUrl?: string
// siteMetadata is free form
[key: string]: unknown
Copy link
Contributor

Choose a reason for hiding this comment

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

Fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean - in context of gatsby core it doesn't matter much, because core doesn't make use of those fields really

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it might matter when we use those types for public index.d.ts declaration instead of hand written one like we do right now

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, doesn't matter at all

Just weird to see those

Copy link
Contributor Author

@pieh pieh May 4, 2020

Choose a reason for hiding this comment

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

I needed it here I think for tests in particular which were setting random thing (really random):

    setSiteConfig({
      someRandomThing: `hi people`,
      plugins: [],
    })

---edit - that was wrong example - see comment below

Copy link
Contributor Author

@pieh pieh May 4, 2020

Choose a reason for hiding this comment

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

above was wrong one - that one was failing - it was for this one:

    const action = setSiteConfig({
      siteMetadata: {
        hi: true,
      },
    })

Copy link
Contributor

Choose a reason for hiding this comment

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

someRandomThing: `hi people`

This is almost as good as your house is on fire

Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

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

Nice work!

@sidharthachatterjee sidharthachatterjee added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label May 4, 2020
@gatsbybot gatsbybot merged commit 62d6bb4 into master May 4, 2020
@delete-merged-branch delete-merged-branch bot deleted the dont-validate-config-in-reducer branch May 4, 2020 16:52
@@ -47,6 +47,9 @@ export interface IGatsbyConfig {
title?: string
author?: string
description?: string
sireUrl?: string
Copy link
Contributor

@muescha muescha May 26, 2020

Choose a reason for hiding this comment

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

typo here... siteUrl

--> #24488

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙈 Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when adding siteMetaData. it won't run
4 participants