-
Notifications
You must be signed in to change notification settings - Fork 468
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
Replace instanceof checks with their respective predicates #1518
Conversation
@@ -157,9 +159,7 @@ export function classDeclarationForOperation( | |||
undefined, | |||
true | |||
); | |||
const isOptional = !( | |||
type instanceof GraphQLNonNull || type instanceof GraphQLNonNull |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol what even was this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shadaj this is way back in history but can you recall if this was intentional or possibly a mistake?
Re: 58f07c4#diff-508d6e2cd0e11941b1548a2ea48a909dR139
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a mistake! I really hope there's no situation in which double-checking a type is actually needed 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shadaj Was removing the second check the mistake, or was that correct? Like this was originally
const isOptional = !(type instanceof GraphQLNonNull || type.ofType instanceof GraphQLNonNull);
was the ofType
incorrectly removed, or was that fine to remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was fine to remove the extra check, I believe that ofType
was eliminated with changes to the IR format so only the top level type needed to be checked.
This replaces all
instanceof GraphQLXYZ
checks with their respective predicate functions.TODO:
*Make sure changelog entries note which project(s) has been affected. See older entries for examples on what this looks like.