Skip to content

RFC Allow recursion within ResolveAbstractType #960

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

Closed
wants to merge 2 commits into from

Conversation

yaacovCR
Copy link
Contributor

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 that this would require a spec change. Alternatively, perhaps the recursion should be considered to be a feature of the internal system itself, possibly limited to JavaScript-like implementations.

This PR provides some potential spec text, were a spec change to be considered necessary.

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.
@netlify
Copy link

netlify bot commented Jun 10, 2022

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 1f434ad
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/62a2ffab4680f800083ac9e3
😎 Deploy Preview https://deploy-preview-960--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.

benjie
benjie previously requested changes Jun 10, 2022
@benjie benjie dismissed their stale review June 10, 2022 08:25

Changes adopted

@leebyron leebyron added the 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) label Jun 16, 2022
@leebyron leebyron changed the title Allow recursion within ResolveAbstractType RFC Allow recursion within ResolveAbstractType Jun 16, 2022
{objectValue}.
- If {possibleRuntimeType} is an Interface type:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on discussion - this probably needs an assertion that {possibleRuntimeType} (maybe {resolvedType}?) implements {abstractType}?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leebyron

Hmmm. This PR allows the internal type system method to resolve to either an object or to a transitive interface. Everything should be fine as long as the internal type system method returns a type that implements the original abstract type, so an assertion is required.

However, the existing behavior also requires a similar assertion, as well as requiring an assertion that the internal method returned an object type. Perhaps to better clarify the proposed change, we should have a separate PR that adds the current assertions/behavior to the spec, and this PR can then modify/loosen those assertions/behavior as desired.

See: #973

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Nov 4, 2024

This proposal is currently in need of a new champion. I am no longer convinced that we need to provide it within the specification, as it is implementable in user land.

@yaacovCR yaacovCR closed this Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants