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

introduce new intersection type #941

Closed
wants to merge 1 commit into from

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Apr 30, 2022

see graphql/graphql-js#3550

[Edit by @benjie]: Explanation/discussion at graphql/graphql-wg#944

@netlify
Copy link

netlify bot commented Apr 30, 2022

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 3d4f410
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/6270daec8e8edf0008a55c82
😎 Deploy Preview https://deploy-preview-941--graphql-spec-draft.netlify.app/draft
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@rivantsov
Copy link
Contributor

sorry, did I miss something, a lot maybe? - where's the discussion? Straight PR into spec?

@yaacovCR
Copy link
Contributor Author

alternative to #939

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Apr 30, 2022

sorry, did I miss something, a lot maybe? - where's the discussion? Straight PR into spec?

This is a continuation of/alternative to the "unions implementing interfaces" work I presented at the last WG.
I started to post some discussion thoughts here but have moved them to: graphql/graphql-wg#944

@@ -343,6 +343,19 @@ UnionTypeExtension :
- extend union Name Directives[Const]? UnionMemberTypes
- extend union Name Directives[Const]

IntersectionTypeDefinition : Description? intersection Name Directives[Const]?
IntersectionMemberTypes?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so IntersectionMemberTypes element is optional? we can have intersection with empty list of types?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is same as Union it could be extended later. So valid to be empty initially in sdl. When validated, it has to be non/empty if I recall correctly, just like unions. But we do allow an intersection with non empty list of member types and currently empty list of possible types due to conflict.

Just like interfaces without implementing types are allowed. I believe :)

claims it returns an Intersection type, it will return only types that are
contained within all of the Intersections's Unions and types that implement all
of the Intersection's Interfaces.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. what is Intersection's Unions? in the grammar rule you mention IntersectionMemberTypes
  2. so... if at runtime field resolver returns a value of type that IS one of union types but does not implement one of interfaces - then what? it is replaced with null? what if field is not nullable? Hell confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At runtime, only concrete object types can be returned as the actual type just like with any other abstract type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And yes, an intersection's unions are its member types that are unions. I copied the grammar from unions where even though object types are the only currently allowed member types, that is not specified at the grammar level, it's specified only as part of validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a non possible type is returned at runtime you get the same error as if you return a non implementing type when using an interface or an object type not in the Union.

In the implementation changes, there was no need to directly touch Execution because the only thing that changes is what goes on in the isSubType method

@yaacovCR
Copy link
Contributor Author

yaacovCR commented May 5, 2022

Summary of discussion from WG https://github.com/graphql/graphql-wg/blob/main/agendas/2022/2022-05-05.md :

union HousePet implements Node was overwhelmingly felt to be best because it is (1) expressive (2) concise (3) less complex (4) will lead to early validation error, which overall would be most valuable. Intersections were felt to be powerful tools that would then propagate a small number of breaking changes to a large number of dangerous and --more importantly-- possibly silent errors!

The action item will then be to move forward as possible with #939 . Immediate next steps is to refine the implementation at graphql/graphql-js#3527 with further checking for all the necessary validation and test changes.

@yaacovCR yaacovCR closed this May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants