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.

Sign up for GitHub

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

Jump to bottom

Update built-in scalar's parseLiteral method to throw TypeError #1827

Merged
merged 1 commit into from
Sep 19, 2019
Merged

Update built-in scalar's parseLiteral method to throw TypeError #1827

merged 1 commit into from
Sep 19, 2019

Conversation

danielrearden
Copy link
Contributor

This addresses #1821

@IvanGoncharov IvanGoncharov added the PR: breaking change 💥 implementation requires increase of "major" version number label Apr 25, 2019
@IvanGoncharov IvanGoncharov added this to the v15.0.0 milestone Apr 25, 2019
@IvanGoncharov IvanGoncharov mentioned this pull request Apr 25, 2019
Closed
IvanGoncharov
IvanGoncharov requested changes Apr 25, 2019
View reviewed changes
src/type/scalars.js Outdated Show resolved Hide resolved
@IvanGoncharov
Copy link
Member

IvanGoncharov commented Apr 29, 2019 •
edited
Loading

Note: enum class also has parseLiteral that needs to be fixed:

graphql-js/src/type/definition.js

Lines 1200 to 1205 in dfcdce7

if (valueNode.kind === Kind.ENUM) {
const enumValue = this.getValue(valueNode.value);
if (enumValue) {
return enumValue.value;
}
}

It can be done in this or in a separate PR.

@danielrearden danielrearden force-pushed the 1821 branch 2 times, most recently from 8eeee77 to 9b57f73 Compare May 3, 2019 13:29
@danielrearden
Copy link
Contributor Author

danielrearden commented May 3, 2019 •
edited
Loading

@IvanGoncharov This PR should be good to go now. 🤞 I can fix GraphQLEnum too (maybe as a separate PR) but I'm lacking a bit of context as to where these methods are actually used. Unlike serialize, as far as I can tell, we only use parseLiteral/parseValue with scalars and never actually call these methods on enum types? Deleting the methods entirely doesn't break any existing tests.

IvanGoncharov
IvanGoncharov reviewed May 6, 2019
View reviewed changes
src/type/scalars.js Outdated
if (ast.kind !== Kind.INT) {
throw new TypeError(
`Int cannot represent non-integer value${
ast.value === undefined ? '.' : ': ' + inspect(ast.value)
Copy link
Member

Choose a reason for hiding this comment

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

since this expression is at the end of the string, I think simple string concatenation with + would be more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

Choose a reason for hiding this comment

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

Thanks

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Jun 7, 2019
@IvanGoncharov
Make 'parseLiteral' optional and use wrapped 'parseValue' by default
c34e984 
Motivation:
graphql#500
graphql#1827 (comment)
@IvanGoncharov IvanGoncharov mentioned this pull request Jun 7, 2019
Merged
IvanGoncharov added a commit that referenced this pull request Jun 7, 2019
@IvanGoncharov
Make 'parseLiteral' optional and use wrapped 'parseValue' by default (#…
f2c3e96 
…1955)

Motivation:
#500
#1827 (comment)
@IvanGoncharov IvanGoncharov changed the title Update built-in scalars' parseLiteral method to throw TypeError Update built-in scalar's parseLiteral method to throw TypeError Sep 19, 2019
@IvanGoncharov IvanGoncharov merged commit dd3b265 into graphql:master Sep 19, 2019
@IvanGoncharov
Copy link
Member

@danielrearden Sorry for the long delay.
We finally started working on 15.0.0 so I merged this breaking change.

@danielrearden danielrearden deleted the 1821 branch September 19, 2019 17:04
@jaydenseric jaydenseric mentioned this pull request Dec 24, 2019
Closed
vladar added a commit to gatsbyjs/gatsby that referenced this pull request Jan 26, 2021
@vladar
gatsby-transformer-sharp: fix incorrect enum value
11f6d01 
GraphQL 15 is stricter with enum values after graphql/graphql-js#1827
vladar added a commit to gatsbyjs/gatsby that referenced this pull request Jan 26, 2021
@vladar
gatsby-plugin-image: fix incorrect enum value
557df4f 
GraphQL 15 is stricter with enum values after graphql/graphql-js#1827
@vladar vladar mentioned this pull request Feb 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IvanGoncharov IvanGoncharov IvanGoncharov requested changes

Assignees
No one assigned
Labels
PR: breaking change 💥 implementation requires increase of "major" version number
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@danielrearden @IvanGoncharov