Skip to content

Conversation

IvanGoncharov
Copy link
Member

No description provided.

// A value must be provided if the type is non-null.
if (isNonNullType(type)) {
if (isNullish(value)) {
if (value == null) {
Copy link
Member Author

Choose a reason for hiding this comment

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

== null block null and undefined but not a NaN and simular values

export default function isNullish(value: mixed): boolean %checks {
return value === null || value === undefined || value !== value;
}

const parser = this._scalarConfig.parseValue;
if (isInvalid(value)) {
return undefined;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Safe because it only used here:

if (isNullish(value)) {
// Explicitly return the value null.
return ofValue(null);
}
if (isScalarType(type)) {
// Scalars determine if a value is valid via parseValue(), which can
// throw to indicate failure. If it throws, maintain a reference to
// the original error.
try {
const parseResult = type.parseValue(value);

So null and undefined values are never passed to parseValue.
Even though it's not breaking anything in graphql-js it's still a minor breaking change since there is a chance that someone using parseValue directly.

@IvanGoncharov IvanGoncharov added the PR: breaking change 💥 implementation requires increase of "major" version number label Jun 5, 2018
Copy link
Contributor

@mjmahone mjmahone left a comment

Choose a reason for hiding this comment

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

I think this makes sense to include in 14.0: as you said, removing the isInvalid => undefined case is a slight breaking change.

@IvanGoncharov IvanGoncharov merged commit ae23c90 into graphql:master Jun 5, 2018
@IvanGoncharov IvanGoncharov deleted the nanTests branch June 6, 2018 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed PR: breaking change 💥 implementation requires increase of "major" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants