graphql-js
graphql-js copied to clipboard
Allow interface resolveType functions to resolve to child interfaces
= The child interfaces must eventually resolve to a runtime object type.
implements: #3253
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...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
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
hi @IvanGoncharov just checking on the status here, no rush, just curious!
good news! i rebased on main and after #3624 , the report uploads correctly!
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
.thencall graphql-jshas 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-jswithout 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.
See: https://github.com/graphql/graphql-spec/pull/960 https://github.com/graphql/graphql-wg/pull/1016