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

Fixup: add Experimental note to variable-directive tests #1458

Merged
merged 1 commit into from
Aug 8, 2018

Conversation

mjmahone
Copy link
Contributor

@mjmahone mjmahone commented Aug 8, 2018

Added a parsing option to the test harness expectPassesRule and expectFailsRule functions.

Also added experimental flags on all tests with variable definition directives.

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Aug 8, 2018

@mjmahone Side note: I reviewed #1437 one more time and I concern that we exposing experimental feature in introspection:

VARIABLE_DEFINITION: {
value: DirectiveLocation.VARIABLE_DEFINITION,
description: 'Location adjacent to a variable definition.',
},

It exposed via printSchema and via introspection query:
"""Location adjacent to a variable definition."""
VARIABLE_DEFINITION

@mjmahone
Copy link
Contributor Author

mjmahone commented Aug 8, 2018

Hmmm that may take a deeper fix. As a note, @leebyron did not have an issue exposing the RFC schema directives in #376 even before we had pushed the GraphQL spec change through (of note, the spec changes weren't merged in until this February, nearly two years later). I think this probably falls into that same category: even if exposed, it's not possible to use unless you add an experimental option in your parser.

Given there seems to be little danger to exposing it in introspection, but exposing it behind a feature flag will require significant re-architecting of introspection, I don't think we should hide it from introspection.

@mjmahone mjmahone merged commit 1ac0bf8 into master Aug 8, 2018
@IvanGoncharov IvanGoncharov deleted the vardefs-test branch August 17, 2018 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants