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

Port parts of Structured Errors to TS + Add two more errors #20597

Merged
merged 15 commits into from
Mar 13, 2020

Conversation

LekoArts
Copy link
Contributor

@LekoArts LekoArts commented Jan 14, 2020

Description

More improvements to our structured errors and also porting some parts over to TS. What has been done:

  • Added types for joi
  • Port gatsby-cli/src/structured-errors over to TS + tests
    • Add ENUMs for types, add two more errors, change message to sourceMessage on 11321
  • Port gatsby/src/query/error-parser over to TS
  • Port gatsby/src/utils/api-runner-error-parser over to TS and change message to sourceMessage

How to test / Screenshots

  1. Using graphql instead of graphql(``) in gatsby-node.js

Before:

errors-graphql-backticks-gatsby-node-js-BEFORE

After:

errors-graphql-backticks-gatsby-node-js-AFTER

  1. Defining a variable in a query but not using it

Before:

unused-variable-BEFORE

After:

unused-variable-AFTER

Documentation

So far no docs changes needed.

Related Issues

Part 2 of #20120

Appendix

My list of ideas of what to fix:

@mathieudutour
Copy link
Contributor

mathieudutour commented Jan 21, 2020

Ah I've just seen this PR. I opened 2 PRs related to this as quick fixes to what I stumbled upon (#20750, #20749). This probably supersedes them (but I don't think it fixes them yet).

@LekoArts LekoArts changed the title Part 2 of Improvements to Structured Errors Port parts of Structured Errors to TS + Add two more errors Jan 22, 2020
@LekoArts LekoArts marked this pull request as ready for review January 22, 2020 14:22
@LekoArts LekoArts requested a review from a team as a code owner January 22, 2020 14:22

interface IConstructError {
details: {
id?: keyof typeof errorMap
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This now allows for auto completion of all error IDs 👍

column: number
}

interface IStructuredError {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This schema is more or less copied from the joi schema

@@ -0,0 +1,20 @@
import errorParser from "../error-parser"

describe(`query-error-parser`, () => {
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 added the describe block so that Webstorm can run it 😬

...cb(matched),
...{ location },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TS was complaining about ...(location && { location }) so I dropped the check. It'll return undefined now. Which is fine as we check for truthy values on that further down the line.

return {
id: `11330`,
context: { message: match[0], arg: match[1] },
context: { sourceMessage: match[0], arg: match[1] },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

message was inconsistent here (The IMatch interface unconvered that) so it was changed to sourceMessage - also in error-map

console.log.mockClear()
process.exit.mockClear()
;(console.log as jest.Mock).mockClear()
;((process.exit as unknown) as jest.Mock).mockClear()
Copy link
Contributor

Choose a reason for hiding this comment

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

lol what a goofy syntax

@ascorbic
Copy link
Contributor

Aside from rebasing, is there anything stopping this being merged?

@LekoArts
Copy link
Contributor Author

Aside from rebasing, is there anything stopping this being merged?

nope.

The PR #20749 should address Michal's comment: #20749 (comment) so it's not blocking for this PR

ascorbic
ascorbic previously approved these changes Mar 13, 2020
@ascorbic ascorbic added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Mar 13, 2020
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.

6 participants