isSpecifiedDirective should not assume Directive type

[Cito]

isSpecifiedDirective should not just check the name property, but also the type

[IvanGoncharov]

@Cito Thanks for PR 👍
I just trying to understand what's the problem with the current check.
Idea is to have non-overlapping checks so if you have value and want to enusure it's a directive you would use:

graphql-js/src/type/directives.js

Lines 32 to 34 in ce832ee

	declare function isDirective(
	directive: mixed,
	): boolean %checks(directive instanceof GraphQLDirective);

Also, note that isSpecifiedDirective take directive of type GraphQLDirective

graphql-js/src/type/directives.js

Lines 190 to 192 in ce832ee

	export function isSpecifiedDirective(
	directive: GraphQLDirective,
	): boolean %checks {

So if you need to test if some value is standard directive you would do:

if (isDirective(value) && isSpecifiedDirective(value)) {
  // ...
}

What do you think?

[Cito]

I just trying to understand what's the problem with the current check.

The problem was that it was unclear to me that this should be considered a non-overlapping, additional check, and not as a standalone check. To me a standalone check would be more intuitive. I was also looking at these four tests which run the predicate against various garbage. These tests should be removed as superfluous and misleading if the input is supposed to be a directive anyway. Otherwise, a test with {name: 'skip'} as garbage should be added as well.

Just had a look at isSpecifiedScalarType for comparison, which is even more unclear, since it makes an isNamedType check internally. When it's supposed to be a non-overlapping check it should assume that it's a ScalarType already, right? And otherwise, it should check more speficifally for ScalarType, not NamedType.

[IvanGoncharov]

I was also looking at these four tests which run the predicate against various garbage.

You added them in #1781 😄

Just had a look at isSpecifiedScalarType for comparison, which is even more unclear, since it makes an isNamedType check internally.

isIntrospectionType also has it 😞

export function isIntrospectionType(type: mixed): boolean %checks {
  return (
    isNamedType(type) &&

The problem was that it was unclear to me that this should be considered a non-overlapping, additional check, and not as a standalone check.

My idea was that type checks (e.g. isInputType, isScalarType, etc.) should be standalone but entity checks (e.g. isSpecifiedDirective) should be guarded by type checks.

But since isIntrospectionType and isSpecifiedScalarType have different symantics it makes sense to change isSpecifiedDirective.

since it makes an isNamedType check internally.

Can you please change it to isScalarType and add tests.

[Cito]

You added them in #1781 😄

Good that git always reminds us of our past sins 😂

I've pushed a new commit that hopefully is better. Both predicates now work as standalone checks.

[Cito]

Btw, there is another issue here that I don't quite understand.

These checks could all be simplified by using specifiedTypes.some(...), but a comment says this is not possible because " %checks needs a simple expression", so I did not make that simplification.

But what exactly would break here when using some()? It compiles fine, and all tests pass. Maybe it would not be usable as a type guard? But I could not come up with a working example, e.g. this one does not work anyway, for example:

function example(d: GraphQLDirective) {
  if (isSpecifiedDirective(d)) {
    const name: 'include' | 'skip' | 'deprecated' = d.name;  // flow error
  }
}

[IvanGoncharov]

@Cito Merged 🎉 Thanks for PR 👍

But what exactly would break here when using some()? It compiles fine, and all tests pass. Maybe it would not be usable as a type guard?

By default flow assumes that function arguments are mutable so it can be sure that type argument wasn't changed before some callback called.
But since we switched to @flow strict that ensure arguments are always const Flow is finally happy.
As an experiment, you can change @flow strict => @flow and see error messages.
Fixed #1848, thanks for investigating this 👍

[Cito]

Thanks for the explanation. Normally you would expect to see more errors when adding "strict", but you're right, in this case the error disappears because with "strict" flow can assume function parameters are const. Setting "strict" seems to pay off, good decision. 👍