graphql-js icon indicating copy to clipboard operation
graphql-js copied to clipboard

Allow interface resolveType functions to resolve to child interfaces

Open yaacovCR opened this issue 3 years ago • 6 comments

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

implements: #3253

yaacovCR avatar May 19 '22 21:05 yaacovCR

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
Latest commit 225cab91bbddf54db933046be569b6f6dc710d4b
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...

QR Code

Use your smartphone camera to open QR code link.

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

netlify[bot] avatar May 19 '22 21:05 netlify[bot]

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

github-actions[bot] avatar May 19 '22 21:05 github-actions[bot]

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

yaacovCR avatar May 25 '22 18:05 yaacovCR

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

yaacovCR avatar Jun 02 '22 18:06 yaacovCR

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:

  1. that method returned a value or promise, and
  2. 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
  3. 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:

  1. working on a spec PR
  2. presenting it at the next WG
  3. adding that my own opinion is that we should implement this in graphql-js without adopting the spec PR, and then
  4. 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.

yaacovCR avatar Jun 08 '22 18:06 yaacovCR

See: https://github.com/graphql/graphql-spec/pull/960 https://github.com/graphql/graphql-wg/pull/1016

yaacovCR avatar Jun 10 '22 07:06 yaacovCR