Skip to content

introduce new intersection type#941

Closed
yaacovCR wants to merge 1 commit intographql:mainfrom
yaacovCR:intersection
Closed

introduce new intersection type#941
yaacovCR wants to merge 1 commit intographql:mainfrom
yaacovCR:intersection

Conversation

@yaacovCR
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Contributor

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

@yaacovCR
Copy link
Copy Markdown
Contributor Author

alternative to #939

@yaacovCR
Copy link
Copy Markdown
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

- extend union Name Directives[Const]

IntersectionTypeDefinition : Description? intersection Name Directives[Const]?
IntersectionMemberTypes?
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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