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

findDangerousChanges: sort fields inside 'defaultValue' #2151

Merged
merged 1 commit into from
Sep 3, 2019

Conversation

IvanGoncharov
Copy link
Member

Fixes #2150

@IvanGoncharov IvanGoncharov added the PR: bug fix 🐞 requires increase of "patch" version number label Sep 2, 2019
@@ -395,6 +396,9 @@ function findArgChanges(
description: `${oldType.name}.${oldField.name} arg ${oldArg.name} defaultValue was removed.`,
});
} else {
// Since we looking only for client's observable changes we should
// compare default values in the same representation as they are
// represented inside introspection.
Copy link
Member Author

Choose a reason for hiding this comment

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

For input type default values we apparently just compare it's stringified value instead of doing a deep comparison.

@Neitsch It's intentional since we want to detect only changes observable by the client.
So we should compare defaultValues in the same representation as used by introspection:

defaultValue: {
type: GraphQLString,
description:
'A GraphQL-formatted string representing the default value for this input value.',
resolve(inputVal) {
const valueAST = astFromValue(inputVal.defaultValue, inputVal.type);
return valueAST ? print(valueAST) : null;
},
},

It's important because defaultValue is always specified in "internal" format (same format as returned by parseLiteral/parseValue and passed into serialize) that mean it can be for example instances of Date class and === wouldn't work for them but if you pass both values into serialize you can compare resulting strings without any problems.

@IvanGoncharov IvanGoncharov merged commit 6609d39 into graphql:master Sep 3, 2019
@IvanGoncharov
Copy link
Member Author

@mrtnzlml @Neitsch Thanks for a quick review 👍

@mrtnzlml
Copy link

mrtnzlml commented Sep 3, 2019

Thanks for the fix. It was really fast! :)

@IvanGoncharov
Copy link
Member Author

@mrtnzlml Released in v14.5.5 🎉 sorry for the delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: bug fix 🐞 requires increase of "patch" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

findDangerousChanges doesn't work as expected after lexicographic sort and deserialization
3 participants