-
-
Notifications
You must be signed in to change notification settings - Fork 809
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
Composite computed fields #5162
Conversation
…mputed field (not sure about unions etc, yet)
…ection set contains remote fields)
🦋 Changeset detectedLatest commit: 3669811 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…nt local yarn version, or Windows platform ...)
Thanks for the PR! |
@asodeur Like |
@MichalLytek yes, that is the plan. As is the PR works in a little in-house PoC but getting all the corner-cases to work required quite some special casing. So there is a lot of clean-up left to do. |
I hope this is ready for a first round of review now. There are some areas that might need better test coverage:
|
T: { | ||
selectionSet: '{ id }', | ||
fieldName: 'byRepresentation', | ||
args: ({ id, value }) => ({ representation: { id, value } }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we expect value
here?
Because it is needed for next
only, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, should have made clear by proper typing value
is optional and will only be filled if available b/c a) value
was explicitly queried, or b) the computed field was queried. Fixed the typing and added a test to show value
will not be filled when it is not required.
…ot a required argument to the entry point and added a test that shows it is not provided when the computed field is not queried.
3d29205
to
2b6e9cc
Compare
Description
This is adding the ability to return non-scalar types from computed fields.
Before this a computed field of non-scalar type would always return null. Now you can do
where
T
is an enum, interface, object, or union type.T
does not need to be merged type. However, all types reachable fromT
will be part of the isolated schema (and potentially the base schema as well).Related #4554
Type of change
Please delete options that are not relevant.
Screenshots/Sandbox (if appropriate/relevant):
Adding links to sandbox or providing screenshots can help us understand more about this PR and take action on it as appropriate
How Has This Been Tested?
Unit tests (eventually will need more ...)
Test Environment:
@graphql-tools/stitch
: 8.7.47Checklist:
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...