Allow interface resolveType functions to resolve to child interfaces #3599

yaacovCR · 2022-05-19T21:09:33Z

= The child interfaces must eventually resolve to a runtime object type.

implements: #3253

netlify · 2022-05-19T21:09:36Z

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	`225cab9`
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/62990d550c2dcb0009fb76e9
😎 Deploy Preview	https://deploy-preview-3599--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

github-actions · 2022-05-19T21:10:05Z

Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

@github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
@github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

src/execution/execute.ts

src/execution/__tests__/abstract-test.ts

src/execution/execute.ts

yaacovCR · 2022-05-25T18:31:54Z

hi @IvanGoncharov just checking on the status here, no rush, just curious!

Requested changes addressed.

src/execution/execute.ts

= The child interfaces must eventually resolve to a runtime object type. = Interface cycles raise a runtime error.

yaacovCR · 2022-06-02T18:28:26Z

good news! i rebased on main and after #3624 , the report uploads correctly!

because it could not be the runtime type(name), could be an intermediate interface I considered just renaming it to resolvedTypeName, but that's confusing, because the variable could hold a promise, and resolvedResolvedTypeName overloads "resolve"

IvanGoncharov

This PR still has some unfinished discussions.
But I discovered one major roadblock, it requires spec PR, because otherwise, it contradicts spec:
https://spec.graphql.org/draft/#sec-Value-Completion.Resolving-Abstract-Types

I suggest opening spec PR that changes the algorithm to be recursive.
+ I will vote for fast-track it to stage2 if you present it on the next WG.

yaacovCR · 2022-06-08T18:33:51Z

Two things:

First, what other discussions are pending?

The two pending issues I had seen were the content of the error messages and whether cycles should be allowed, and I have changed this PR to correspond to your suggestions. Are you not sure of the optimal behavior with respect to one or both of these issues? Do you feel that you need a certain amount of time to think about these issues, or do these issues need feedback from the graphql-js-wg or the main graphql-wg? I will address your comment in a second, but we can work on these issues in parallel, as although async discussions have their drawback, they do allow us to open up multiple threads and make progress in parallel.

Second, I think this PR does not contradict spec, as the spec says we can resolve by whatever means we want.

In particular, consider the evolution of this PR. A previous version had a separate resolve method corresponding to the ResolveAbstractType step you have linked. But:

that method returned a value or promise, and
refactoring any functionality returning a promise or value into a separate function forces the calling function to perform an additional isPromise check and .then call
graphql-js has the aspirational policy of not only not entering the task queue unnecessarily, but of never adding an additional job to job queue (see https://blog.greenroots.info/task-queue-and-job-queue-deep-dive-into-javascript-event-loop-model and https://developer.mozilla.org/en-US/docs/Web/API/HTML_DOM_API/Microtask_guide/In_depth#run_javascript_run)

And so we had to mix the separate resolveAbstractType method with abstract value completion using recursive completeAbstractValue and completeAbstractValueImpl inner functions and an incomplete resolveAbstractType portion linking between the two.

So, this change does not contradict the spec, but the performance (?) constraints of graphql-js have causes us to make a change that makes the one-to-one match between the implementation and the spec more difficult to grasp.

[Perhaps there are 3 tenets of graphql-js, not 2!: (1) a reference implementation, (2) a production-ready JavaScript graphql pipeline, (3) a fully-fleshed out exercise in avoiding any extra jobs in the job queue. And perhaps there is some sort of inchoate rule where for any given issue, only two of the three graphql-js tenets may be upheld!]

Bottom line, I don't think we should necessarily force all complying implementations to implement this feature in JavaScript. I think it's a useful feature, but I don't know that in every language it should be mandated.

However, I am fine:

working on a spec PR
presenting it at the next WG
adding that my own opinion is that we should implement this in graphql-js without adopting the spec PR, and then
accepting whatever happens

Although please see above -- let's work out the other issues in the meantime. I am leaving this as open, please follow-up when possible.

@IvanGoncharov

This formalizes the proprosed feature within `graphql-js` whereby the internal method provided by JavaScript for runtime type resolution is allowed to return an intermediate interface. See: Issue: graphql/graphql-js#3253 PR: graphql/graphql-js#3599 @IvanGoncharov [suggested](graphql/graphql-js#3599 (review)) that this would require a spec change. Alternatively, perhaps the recursion [should be considered to be a feature of the internal system](graphql/graphql-js#3599 (comment)) itself, possibly limited to JavaScript-like implementations. This PR provides some potential spec text, were a spec change to be considered necessary.

yaacovCR · 2022-06-10T07:36:46Z

See:
graphql/graphql-spec#960
graphql/graphql-wg#1016

leebyron · 2022-07-07T19:24:53Z

src/execution/execute.ts

      { nodes: fieldNodes },
    );
  }

-  if (!isObjectType(runtimeType)) {
+  if (isInterfaceType(possibleRuntimeType)) {


what about union types?

Nevermind - based on feedback from WG discussion, a union type shouldn't be possible (though does this get potentially complicated by the proposal for union constraints via implementing interfaces?

Yes, if unions ever implement interfaces, they could potentially be a link in the chain!

leebyron · 2022-07-07T21:16:19Z

src/execution/execute.ts

    throw new GraphQLError(
-      `Abstract type "${returnType.name}" must resolve to an Object type at runtime for field "${info.parentType.name}.${info.fieldName}". Either the "${returnType.name}" type should provide a "resolveType" function or each possible type should provide an "isTypeOf" function.`,
+      `Abstract type "${abstractType.name}" must resolve to an Object type or an intermediate Interface type at runtime for field "${info.parentType.name}.${info.fieldName}". Either the "${abstractType.name}" type should provide a "resolveType" function or each possible type should provide an "isTypeOf" function.`,


What do you think about only altering this error message in the case that an intermediate Interface type actually exists as a potential case?

Looking at the unit test impact, my sense is the primary downside of this change is the additional complication to this error message that makes it slightly harder to understand.

Ideally for the large majority common case where there is no intermediate interface, we don't burden the error message

👍 Tailoring the error message to the interface at hand seems pretty reasonable.

yaacovCR added the PR: feature 🚀 requires increase of "minor" version number label May 19, 2022

saihaj reviewed May 19, 2022

View reviewed changes

src/execution/execute.ts Outdated Show resolved Hide resolved

saihaj requested a review from a team May 20, 2022 01:04

IvanGoncharov requested changes May 22, 2022

View reviewed changes

src/execution/execute.ts Outdated Show resolved Hide resolved

src/execution/execute.ts Outdated Show resolved Hide resolved

IvanGoncharov requested changes May 22, 2022

View reviewed changes

src/execution/__tests__/abstract-test.ts Outdated Show resolved Hide resolved

src/execution/__tests__/abstract-test.ts Outdated Show resolved Hide resolved

src/execution/__tests__/abstract-test.ts Outdated Show resolved Hide resolved

src/execution/execute.ts Outdated Show resolved Hide resolved

yaacovCR force-pushed the resoooolve branch 3 times, most recently from 404f492 to 81ebe2a Compare May 22, 2022 20:54

IvanGoncharov previously requested changes May 23, 2022

View reviewed changes

src/execution/execute.ts Outdated Show resolved Hide resolved

yaacovCR requested review from a team and IvanGoncharov May 31, 2022 03:31

yaacovCR force-pushed the resoooolve branch 4 times, most recently from 0d7cdd3 to 74d5712 Compare June 1, 2022 20:24

yaacovCR requested review from a team and removed request for a team June 1, 2022 20:26

yaacovCR force-pushed the resoooolve branch 3 times, most recently from 2dc39ce to 4d153df Compare June 2, 2022 02:38

yaacovCR commented Jun 2, 2022

View reviewed changes

src/execution/execute.ts Outdated Show resolved Hide resolved

yaacovCR added 5 commits June 2, 2022 21:12

Allow interface resolveType functions to resolve to child interfaces

a30daec

= The child interfaces must eventually resolve to a runtime object type. = Interface cycles raise a runtime error.

change function name

d89b1ae

use variables for abstract types in tests

00dbea1

fix chain and add test

7167fcf

standardize order

81c5ed9

IvanGoncharov added 2 commits June 2, 2022 21:12

remove unneeded check for cycles

6c8ad45

review change

4dbfe20

yaacovCR force-pushed the resoooolve branch from 702bc07 to 4dbfe20 Compare June 2, 2022 18:12

yaacovCR added 3 commits June 2, 2022 21:29

change inner recursive function to completeAbstractValueImpl

5ba43d8

re-align error message as possible to original

9e0c4d0

IvanGoncharov requested changes Jun 8, 2022

View reviewed changes

yaacovCR mentioned this pull request Jun 10, 2022

RFC Allow recursion within ResolveAbstractType graphql/graphql-spec#960

Open

leebyron reviewed Jul 7, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow interface resolveType functions to resolve to child interfaces #3599

Allow interface resolveType functions to resolve to child interfaces #3599

yaacovCR commented May 19, 2022 •

edited

Loading

netlify bot commented May 19, 2022 •

edited

Loading

github-actions bot commented May 19, 2022

yaacovCR commented May 25, 2022

yaacovCR commented Jun 2, 2022

IvanGoncharov left a comment •

edited

Loading

yaacovCR commented Jun 8, 2022

yaacovCR commented Jun 10, 2022

leebyron Jul 7, 2022

leebyron Jul 7, 2022

yaacovCR Jul 7, 2022

leebyron Jul 7, 2022

yaacovCR Jul 7, 2022

Allow interface resolveType functions to resolve to child interfaces #3599

Are you sure you want to change the base?

Allow interface resolveType functions to resolve to child interfaces #3599

Conversation

yaacovCR commented May 19, 2022 • edited Loading

netlify bot commented May 19, 2022 • edited Loading

✅ Deploy Preview for compassionate-pike-271cb3 ready!

github-actions bot commented May 19, 2022

yaacovCR commented May 25, 2022

yaacovCR commented Jun 2, 2022

IvanGoncharov left a comment • edited Loading

Choose a reason for hiding this comment

yaacovCR commented Jun 8, 2022

First, what other discussions are pending?

Second, I think this PR does not contradict spec, as the spec says we can resolve by whatever means we want.

yaacovCR commented Jun 10, 2022

leebyron Jul 7, 2022

Choose a reason for hiding this comment

leebyron Jul 7, 2022

Choose a reason for hiding this comment

yaacovCR Jul 7, 2022

Choose a reason for hiding this comment

leebyron Jul 7, 2022

Choose a reason for hiding this comment

yaacovCR Jul 7, 2022

Choose a reason for hiding this comment

yaacovCR commented May 19, 2022 •

edited

Loading

netlify bot commented May 19, 2022 •

edited

Loading

IvanGoncharov left a comment •

edited

Loading