Can you just do:

for (const directive of directives) {
  const directiveName = directive.name.value;
  if (repeatableMap[directiveName]) {
    continue;
  }
  // ...
}

I guess I generally trying to avoid usage of break and continue, but I can update the code and use it here.

@OlegIlyenko Thinking about this can you change repeatableMap to uniqueDirectiveMap with reverse value and do:

for (const directive of directives) {
  const directiveName = directive.name.value;
  if (uniqueDirectiveMap[directiveName]) {
    // ...
  }
}

Plus uniqueDirectiveMap match nicely with UniqueDirectivesPerLocation rule name.

good idea, done ae9e7f9

In JS you can do that with a single config.repeatable == null and we already using it in couple places.

I believe config.repeatable === undefined is still necessary since I defined the flow type like this:

repeatable?: ?boolean,

If I don't do the check, I will get following flow error:

Cannot assign `config.repeatable === null ? false : config.repeatable` to `this.repeatable` because:
 - null or undefined [1] is incompatible with boolean [2].
 - undefined [1] is incompatible with boolean [2].

@OlegIlyenko Note == null (two equal signs) compares both to null and undefined and Flow should handle it without problems.

If I don't do the check, I will get following flow error:

It's because you use === instead of ==.
It's safe and very common pattern in JS to do == null:
https://eslint.org/docs/rules/eqeqeq#smart

Ah, I see. Haven't noticed this, thanks for the clarification.

I got an impression that the use of == is discouraged in javascript community since it relies of a set of coercion rules that many people find error-prone. This is why I tried to avoid using it. I was not aware of the exceptions described in the eslint documentation though. I will update the code and use ==.

Up to discussion: I don't think unknown directives should be treated as non-repeatable. Unknown directives should trigger KnownDirectives and other rules shouldn't make any assumption about it.
Previously it was safe since all directives were non-repeatable.

It is a good point. After thinking about it, I think I would agree with you. In both scenations: directive is not known and it is known but repeatable, it should be excluded from this validation entirely. I will adjust the logic.

@OlegIlyenko As discussed on the working group we can't assume that legacy servers would support repeatable. So this change will break GraphiQL and many other tools.

I would suggest making option directiveRepeatable similar to this one:

Long term solution would be to implement two-stage introspection as suggested by Lee.

It's a good point, I added an option for it 3b5bf03 I also made if disabled by defult in order to maintain backwards compatibility.

Also this one: 3a7fafa I forgot to update the introspection flow type

@OlegIlyenko Can you please address this comment in spec RFC:
https://github.com/facebook/graphql/pull/472/files/1ba4e196aa710a10eceaa346d02a970dfd0b2d3a#diff-35fb422eded6e8f4df638c0d159008f6

It blocks this PR but I think we should discuss it in the scope of spec RFC.

Could you please describe in more detail what kind of information you would like to add? I'm not 100% sure I understood your comment.

@OlegIlyenko I mean if merge this PR that will expose repeatable through introspection and it would be significantly harder to rename it later.
As I wrote in my comment to the spec RFC I think this field should be named isRepeatable for consistency with isDeprecated.

Everything else in this PR looks good so would be great to discuss this last point but since this PR is just an implementation of spec RFC I think we should discuss it in spec PR.

I see, thanks for the clarification. Sure thing, I will open a discussion on this topic in the RFC PR.

@OlegIlyenko I feel very stupid because in the first comment of this thread I linked review comment I forget to submit (review comments UI is very weird).
I already submitted it in.

👍 i see, thanks! now it's clear, i was confused about wrong link

I did the renaming 56fee7d I will adjust the spec text accordingly

Oh I just saw this comment. Sorry for the late comment: I guess keeping consistent with isDeprecated makes sense, though it still feels a little weird. Oh well.

I wonder if this should be:

+repeatable?: boolean,

If someone never uses repeatable directives, this adds no extra weight to the node.

I also don't think we need the is prefix: the fact that it's a boolean is enough information, and the question repeatable? false is close-to-valid English as-is (just as understandable as is repeatable? false).

The is prefix primarily added for consistency with isDeprecated, as @IvanGoncharov pointed out https://github.com/facebook/graphql/pull/472/files/1ba4e196aa710a10eceaa346d02a970dfd0b2d3a#diff-35fb422eded6e8f4df638c0d159008f6

I think I would prefer it keep it mandatory. This makes it easier to work with and i feel that the performance overhead is negligible. But I can also make it optional.

WDYT?

@OlegIlyenko In the context of AST, I'm more in favor of @mjmahone proposal since we already have block with similar prototype:

Plus it looks like JS AST doesn't use is prefix, e.g. async:
https://github.com/estree/estree/blob/df2290b23a652e1ec354b6775459a3b42e22f108/es2017.md#function

But outside of AST I strongly believe it should be named isRepeatable for the consistency with isDeprecated.

This makes it easier to work with and i feel that the performance overhead is negligible.

I'm with you on that point especially since JS parser always preserve boolean flags and they typically work with way bigger files than GraphQL.
Also defacto (in our parser) we always provide values for all keys so why should Flow typings mark that as optional.

That said I think it's outside of scope of this PR and should be disscussed for AST as a whole and not for individual flags.
So for now I think it should be optional.

I think both alternatives should be fine. I also considered keep repeatable name on AST nodes, but then renamed it with is prefix across the whole codebase for the sake of the naming consistency.

But I think it should totally fine to have repeatable field on AST node and then use isRepeatable everywhere else. @mjmahone What do you think?

I renamed isRepeatable back to repeatable in all places except introspection: 89f5780

This is technically a breaking change. It's probably OK as I doubt anyone is using this validation explicitly yet, but definitely need to note that in our release notes.

But I think there should be a way, today, to re-order our validation contexts to provide a Schema SDL if possible. I will probably end up forking all of these validation rules to run off an AST-only schema object, because building (and extending) the current GraphQLSchema object is too slow. I won't block this on not using the GraphQLSchema, but I'd like for us to move in that direction.

Do understand it correctly: you mean expressing all existing validations purely in terms of SDL context? Does this also imply that in case of normal query validation you would like to transform existing schema in SDL and then validate against it?

@mjmahone It's not a breaking change since ASTValidationContext is not a part of public API. As we agree during 14.0.0 SDL validation is an experimental feature so I didn't expose this and other new class as public API.
ATM only ValidationContext is exposed so if library client use only public API he shouldn't be affected by this change in any way.

Why would this default to false?

For example, if it would be true, then after an update without any adjustments third-party libraries (like GraphiQL) would start to send queries with isRepeatable flag to the servers that potentially not updated yet. So default is primarily defined like this to maintain backwards compatibility.

Would you like to change the default and require all downstream libraries to explicitly set this flag? (at least at the initial stage where servers are not yet updated)

@mjmahone Do you remember we discussed it on the last WG in the scope of my
"schema description" RFC. And the consensus was to keep it backward compatible for now but design two stage introspection as a long-term solution.