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

Support @oneOf directive spec #3768

Open
2 of 7 tasks
acao opened this issue Sep 6, 2024 · 5 comments
Open
2 of 7 tasks

Support @oneOf directive spec #3768

acao opened this issue Sep 6, 2024 · 5 comments

Comments

@acao
Copy link
Member

acao commented Sep 6, 2024

The @oneOf spec is well past the level of maturity we require, and has already made it's way into graphql-js releases 😍 Input polymorphism was the first goal I had when joining the graphql project in 2018, and the spec developers have done a wonderful job of advancing this and it's predecessors!

We just need to introduce support in a few places to enable this spec across our monorepo.

Cross-ecosystem User Outcomes

  • Should see @oneOf in completion as a built-in directive on Input Type Definitions (can we make all graphql-js spec directives always show up where they should be automatically somehow? currently each spec directive is added to completion manually iirc)
  • Completion for @oneOf enabled operation arguments should show all available fields at first as it does currently, but if a user tries to add more fields for the input type, no completion appears.
  • Hover for input types themselves, on field argument names for oneOf input types, on input type variables and input type values should all show an indicator of some kind that this is a @oneOf input type. I hope that this might prevent any confusion with the input type argument completion, for users who aren't familiar with @oneOf yet and/or aren't aware that input types are @oneOf in the schema, which could be likely if their work is client focused.
  • There should also be validation for the input arguments, which seems to be an graphql-js rule that needs to be written (may contrib for this effort!) (correct me if I'm wrong!)
  • variablesToJsonSchema() which is used for monaco-graphql and will be used for codemirror 6 needs to support @oneOf via... you guessed it... oneOf 😆 which we are putting the finishing touches on for codemirror-json-schema for the cm6 graphql experience!

GraphiQL-specific User Outcomes

  • Documentation explorer shows that an Input Type is @oneOf
  • graphiql-explorer should honor @oneOf when building queries as well? I have a PR to merge the onegraph graphiql-explorer into our repo and convert it to typescript. They said it was ok to do, but not reccomended, but in the absence of any new plugin to replace it, and with additional a11y needs, it's likely this will happen by graphiql@5.0.0 if not sooner, thus we should be able to add this as well
@yaacovCR
Copy link
Contributor

yaacovCR commented Sep 6, 2024

  • There should also be validation for the input arguments, which seems to be an graphql-js rule that needs to be written

Quick note: oneOf support was added to ValuesOfCorrectTypeRule by @erikkessler1 — see graphql/graphql-js#3513

@acao
Copy link
Member Author

acao commented Sep 6, 2024

@yaacovCR ah great, I assumed I had just overlooked this, keyword search failed me!

@acao
Copy link
Member Author

acao commented Sep 8, 2024

Having a strange issue with this rule in some cursory testing, whilst trying to get validation working
As you can see, on execution, graphql-http is applying the rule correctly.
However if I apply the rule in getDiagnostics() in the universal language service for the same document, no errors are returned from validate() (indicated by the console output, and you can see the rule is being applied as #23 here)

image

  const errors = validate(schema, ast, rules);
  console.log({ errors, rules });

using this version: graphql@16.9.0

@benjie
Copy link
Member

benjie commented Sep 9, 2024

can we make all graphql-js spec directives always show up where they should be automatically somehow? currently each spec directive is added to completion manually iirc

require('graphql').specifiedDirectives has a list of the specified directives; you'd then need to filter these based on the relevant locations.

graphiql-explorer should honor @OneOf when building queries as well?

One thing to keep in mind during this is that even when all fields are nullable variables, val: { a: $a, b: $b } is not valid if val is a @oneOf. This will require a different style of input in explorer, or alternatively when the checkbox is checked for one of the fields, the previous one should be automatically unchecked. Using a radio box rather than a check box might be even clearer.

@acao
Copy link
Member Author

acao commented Sep 9, 2024

@benjie awesome! these directives work automatically for now because of introspection, but I will keep that in mind if we need the actual directive definition types in the future. when i wrote this I had forgotten this!

This will require a different style of input in explorer, or alternatively when the checkbox is checked for one of the fields, the previous one should be automatically unchecked. Using a radio box rather than a check box might be even clearer.

great points! i was thinking something like the latter. for now I can make a PR if they have time to merge it. we really need to take over or replace explorer as there are some huge a11y issues that need addressed as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants