Skip to content

Commit

Permalink
Comments on InputUnion RFC (#817)
Browse files Browse the repository at this point in the history
* Comments on the proposal

Hi, thanks for this extensive and in-depth work.

I will try to keep my comments concise (which might make them sound agressive, apologies if that's the case, it's definitely not the intent)

This PR touches one of the most challengeable founding philosophical choices of GraphQL: having 2 different type systems for input and output.

I understand that the sender -> receiver relation is an unbalanced one: the sender has obligations, the receiver has options.

However:
 - ALL GraphQL users use programming languages (Javascript, Java, C#, Python...) which make no difference between input and output types
 - I actually don't know any other protocol that does that (is there one?)
 - having 2 different type systems for input and output solves (does it?) an implementer problem, not a user problem

The lack of polymorphism on input is only a side-effect of the aformentioned original choice.
In an unreal world, rather than tweak GraphQL to fix that, it would be time for GraphQL2, unifying input and output types (amongst other improvements).

That's very unlikely to happen, but saying so helps forming an opinion on the various proposals:
 - adding yet another polymorphic construct only available on input 'smells' like increasing confusion 
 - it would increase the gap between input and output type systems, rather than reduce it

From there, I think proposal #5 @OneOf is the most useful one:
 - it acts as a constraint on existing type constructs rather than yet another type construct
 - it expresses the required behavior much better than proposal #7

* Comments on the proposal

Hi, thanks for this extensive and in-depth work.

I will try to keep my comments concise (which might make them sound agressive, apologies if that's the case, it's definitely not the intent)

This PR touches one of the most challengeable founding philosophical choices of GraphQL: having 2 different type systems for input and output.

I understand that the sender -> receiver relation is an unbalanced one: the sender has obligations, the receiver has options.

However:
 - ALL GraphQL users use programming languages (Javascript, Java, C#, Python...) which make no difference between input and output types
 - I actually don't know any other protocol that does that (is there one?)
 - having 2 different type systems for input and output solves (does it?) an implementer problem, not a user problem

The lack of polymorphism on input is only a side-effect of the aformentioned original choice.
In an unreal world, rather than tweak GraphQL to fix that, it would be time for GraphQL2, unifying input and output types (amongst other improvements).

That's very unlikely to happen, but saying so helps forming an opinion on the various proposals:
 - adding yet another polymorphic construct only available on input 'smells' like increasing confusion 
 - it would increase the gap between input and output type systems, rather than reduce it

From there, I think proposal #5 @OneOf is the most useful one:
 - it acts as a constraint on existing type constructs rather than yet another type construct
 - it expresses the required behavior much better than proposal #7

* Update InputUnion.md

* clean up merge dirt
  • Loading branch information
ericvergnaud committed Sep 2, 2021
1 parent 8db7763 commit 04cd832
Showing 1 changed file with 26 additions and 0 deletions.
26 changes: 26 additions & 0 deletions rfcs/InputUnion.md
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,17 @@ When an invalid schema or invalid query are used, it should be obvious what went

Criteria score: 🥉

## 🎯 Q. No new polymorphic type construct should be introduced

The lack of polymorphism on input is only a side-effect of having 2 different type systems for input and output, a somewhat confusing GraphQL specificity (all mainstream programming language and API protocol use the same types for input and output).
Adding a new construct for polymorphism support on input 'smells' like increasing confusion, and would increase the gap between input and output type systems, rather than reduce it.

| [1][solution-1] | [2][solution-2] | [3][solution-3] | [4][solution-4] | [5][solution-5] | [6][solution-6] | [7][solution-7]
|----|----|----|----|----|----|----|
| 🚫️ | 🚫️ || 🚫️ ||| 🚫️ |

Criteria score: 🥇

# 🚧 Possible Solutions

The community has imagined a variety of possible solutions, synthesized here.
Expand Down Expand Up @@ -522,6 +533,8 @@ type Mutation {
* ✅ Adding or removing an input type to a union has no extraneous effects on schema design
* [P. Error states and messages should be clear and helpful][criteria-p]
* ✅
* [Q. No new polymorphic type construct should be introduced][criteria-q]
* 🚫️ ``ìnputunion```is a new type construct

## 💡 2. Explicit configurable Discriminator field

Expand Down Expand Up @@ -636,6 +649,8 @@ inputunion AnimalInput @discriminator(field: "species") =
* Reusing input types in multiple input unions can become unwieldy
* [P. Error states and messages should be clear and helpful][criteria-p]
*
* [Q. No new polymorphic type construct should be introduced][criteria-q]
* 🚫️ ``ìnputunion```is a new type construct

## 💡 3. Order based discrimination

Expand Down Expand Up @@ -727,6 +742,8 @@ type Mutation {
* ❔ Not evaluated
* [P. Error states and messages should be clear and helpful][criteria-p]
* ⚠️ Order-based discrimination can lead to some subtle issues based on when one type is chosen over another.
* [Q. No new polymorphic type construct should be introduced][criteria-q]
* ✅ No new construct

## 💡 4. Structural uniqueness

Expand Down Expand Up @@ -822,6 +839,8 @@ input DogInput {
* 🚫 Input types with similar fields may not be able to be combined without breaking changes
* [P. Error states and messages should be clear and helpful][criteria-p]
* 🚫 Structural uniqueness checks are very complex and have many hard-to-describe failure states.
* [Q. No new polymorphic type construct should be introduced][criteria-q]
* 🚫️ ``ìnputunion```is a new type construct

## 💡 5. One Of (Tagged Union)

Expand Down Expand Up @@ -904,6 +923,8 @@ type Mutation {
* ✅ Adding or removing input types to a tagged union requires no extraneous effort
* [P. Error states and messages should be clear and helpful][criteria-p]
* ✅
* [Q. No new polymorphic type construct should be introduced][criteria-q]
* ✅

### Summary of spec changes

Expand Down Expand Up @@ -1026,6 +1047,8 @@ https://github.com/graphql/graphql-spec/pull/733
* ✅ Adding or removing member fields to a tagged type requires no extraneous effort and has no non-local consequences
* [P. Error states and messages should be clear and helpful][criteria-p]
* ✅
* [Q. No new polymorphic type construct should be introduced][criteria-q]
* 🚫️ ``tagged```is a new type construct

### Summary of spec changes

Expand Down Expand Up @@ -1063,6 +1086,7 @@ A quick glance at the evaluation results. Remember that passing or failing a spe
| [N][criteria-n] 🥈 | ✅⚠️ | ✅⚠️ | ✅⚠️ | ✅⚠️ | ✅ | ? | ✅⚠️ |
| [O][criteria-o] 🥈 | ✅️ | 🚫️ | ❔ | 🚫 | ✅ | ? | ✅ |
| [P][criteria-p] 🥉 | ✅️ | ✅️ | ⚠️ | 🚫 | ✅ | ❔ | ✅ |
| [Q][criteria-q] 🥉 | 🚫 | 🚫 | ✅️ | 🚫 | ✅ | ❔ | 🚫 |

[criteria-a]: #-a-graphql-should-contain-a-polymorphic-input-type
[criteria-b]: #-b-input-polymorphism-matches-output-polymorphism
Expand All @@ -1080,6 +1104,7 @@ A quick glance at the evaluation results. Remember that passing or failing a spe
[criteria-n]: #-n-existing-code-generated-tooling-is-backwards-compatible-with-introspection-additions
[criteria-o]: #-o-unconstrained-combination-of-input-types-to-unions
[criteria-p]: #-p-error-states-and-messages-should-be-clear-and-helpful
[criteria-q]: #-q-no-new-polymorphic-type-construct-should-be-introduced

[solution-1]: #-1-explicit-__typename-discriminator-field
[solution-2]: #-2-explicit-configurable-discriminator-field
Expand All @@ -1100,3 +1125,4 @@ proposed as an evolution of Solution 5, and is currently the leading solution.
* ~~[1][solution-1]~~
* ~~[2][solution-2]~~
* ~~[3][solution-3] / [4][solution-4]~~

0 comments on commit 04cd832

Please sign in to comment.