graphiql icon indicating copy to clipboard operation
graphiql copied to clipboard

Support `@oneOf` directive spec

Open acao opened this issue 1 year ago • 6 comments

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

  • [x] 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)
  • [x] 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 [email protected] if not sooner, thus we should be able to add this as well

acao avatar Sep 06 '24 01:09 acao

  • 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 https://github.com/graphql/graphql-js/pull/3513

yaacovCR avatar Sep 06 '24 05:09 yaacovCR

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

acao avatar Sep 06 '24 14:09 acao

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: [email protected]

acao avatar Sep 08 '24 09:09 acao

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.

benjie avatar Sep 09 '24 02:09 benjie

@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

acao avatar Sep 09 '24 17:09 acao

It seems that the RFC is accepted and can be used, see this comment https://github.com/graphql/graphql-spec/pull/825/#issuecomment-2945309875

ruudk avatar Jul 09 '25 09:07 ruudk