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

formatError: put all extensions inside 'extensions' property #1284

Merged
merged 2 commits into from
Apr 6, 2018

Conversation

IvanGoncharov
Copy link
Member

Mirrors graphql/graphql-spec#407

This is breaking change, however, extensions is the relatively new feature that was added in 0.12.0.
Also, it could be easily workaround by providing formatError property to a server:
https://github.com/graphql/express-graphql#options
https://github.com/apollographql/apollo-server#options

Unintended changes: I didn't want to add extensions: undefined to every error inside tests so I refactored them.

Copy link
Contributor

@leebyron leebyron left a comment

Choose a reason for hiding this comment

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

This is great - just some small suggestions inline.

We'll also need some kind of adoption strategy to help users with the breaking change

message: error.message || 'An unknown error occurred.',
locations: error.locations,
path: error.path,
extensions: error.extensions,
Copy link
Contributor

Choose a reason for hiding this comment

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

The extensions key should only exist if there are extensions, so perhaps this should if (error.extensions) and return different shapes

// Extensions
+[key: string]: mixed,
};
+extensions: { [key: string]: mixed } | void,
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on spec description, this should be:

+extensions?: { [key: string]: mixed }

@IvanGoncharov
Copy link
Member Author

@leebyron Thanks for review 🎉

This is great - just some small suggestions inline.

Fixed in 80ca852. I tried dozens of ways how to initialize optional non-undefined readonly property and it's the simplest one I found.

We'll also need some kind of adoption strategy to help users with the breaking change

JSON.stringify(error) is not affected.
As for formatError, if we assume people read Release notes before updating to the new version than it's easy. You just need to write that in GraphQL server of your choice express-graphql, apollo-server, etc. you need to supply formatError as:

import { formatError as baseFormatError, /* ... */ } from 'graphql';

{
  // other options
  formatError(error) {
    const { extensions, ...rest } = baseFormatError(error);
    return { ...extensions, ...rest };
  },
}

@leebyron
Copy link
Contributor

leebyron commented Apr 5, 2018

Nice - Let's get #1305 landed first and I'll let you rebase over that, otherwise I really like this change.

@IvanGoncharov
Copy link
Member Author

@leebyron I rebased this PR on top of master.

@leebyron
Copy link
Contributor

leebyron commented Apr 6, 2018

Great work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants