Add support for leading vertical bar in union types #320
Conversation
spec/Section 3 -- Type System.md
Outdated
| @@ -755,6 +755,22 @@ Union types have the potential to be invalid if incorrectly defined. | |||
| Similarly, wrapping types may not be member types of a Union. | |||
| 2. A Union type must define one or more unique member types. | |||
|
|
|||
| #### Union type syntax | |||
|
|
|||
| Union types are a list of types joined with a vertical bar `|` | |||
There was a problem hiding this comment.
How about: "Union types are defined by delimiting one or more types with a single vertical bar character |:"
spec/Section 3 -- Type System.md
Outdated
| union SearchResult = Photo | Person | ||
| ``` | ||
|
|
||
| You may also use a leading vertical bar |
There was a problem hiding this comment.
We should probably mention the treatment of a trailing vertical bar.
|
@robzhu changes made |
spec/Section 3 -- Type System.md
Outdated
|
|
||
| ```graphql | ||
| union SearchResult = | ||
| | Photo | ||
| | Person | ||
| ``` | ||
|
|
||
| Note Trailing vertical bars are not supported. |
There was a problem hiding this comment.
I would update this to say "Trailing delimiters are not supported:" followed by an (invalid) example. You can "```!graphql" to designate an invalid example.
spec/Section 3 -- Type System.md
Outdated
|
|
||
| ```graphql | ||
| union SearchResult = Photo | Person | ||
| ``` | ||
|
|
||
| You may also use a leading vertical bar | ||
| You may also use a leading vertical bar. |
There was a problem hiding this comment.
Change this to You may also use a leading vertical bar: to be consistent with the opening clause.
|
@alexandrebodin @robzhu But IDL is not merged yet 😱 , see #90. Moreover, this is a syntax change in IDL and it has nothing to do with |
|
@IvanGoncharov good point. Just checked with @leebyron as well. My mistake, I wasn't fully caught up on the status of the rfc-idl branch. @alexandrebodin, can you please re-submit the PR against the rfc-idl branch? |
|
@leebyron @robzhu I think this PR gives a good example of what happens if you don't give enough time for the community to provide feedback before merging. With implementations in a few dozen of languages and a huge number of tools/libraries around GraphQL there are many developers who can provide valuable feedback. Moreover, they are subscribed for updates in this repo: Can we make a rule that PRs with changes beyond grammar/formatting fixes will not be merged in less than 48 hours? That way everyone will have a chance to review it and provide feedback. |
|
@IvanGoncharov that sounds very reasonable. We should probably add a |
|
It's also not clear when these changes take effect? Would this be only in the next "version" of GraphQL, or is merging the PR an immediately binding decision that needs to be supported by all implementations ASAP? |
|
@stubailo In retrospect, it's clear this PR should have been made against the rfc-idl repo. It makes sense to me that we would align these spec updates with batches to coincide with the working group schedule (TBD). Do you have any ideas you think would work well here? |
|
I think having a clear distinction between "changes" and "clarifications", and making sure that changes are merged into a non-deployed "next version" branch, would be enough! Then, having a clear schedule around releases or a warning process when a new release is about to come out, will also help a lot. So that all server/client authors can have some time to update things if they want to. |
@robzhu Great idea 👍 It would make sense to add other unwritten rules, e.g. reference implementation should be updated to match PR before it will be merged. Probably it would make sense to describe it in form of typical workflow of PR in this repo.
Less so, because it more like a test ground and accepting something there doesn't mean it will be merged into spec itself. My idea of ideal workflow is like that:
But maybe it's too much of bureaucracy |
|
I do think merging public-facing stuff into the reference implementation which is not spec compliant is an issue, although a minor one, since there is a very real risk of people coding their API to GraphQL.js, and using things that don't ever end up in the spec. I don't think this issue is related though because the whole SDL isn't in the spec, but if the SDL were in the spec I'd be against adding random changes to it in GraphQL.js. |
This spec change is matched with a PR on the language repository
PR on the parser graphql/graphql-js#907