Skip to content

Split up the completeValue function #311

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

Merged
merged 5 commits into from
Mar 23, 2016

Conversation

JeffRMoore
Copy link
Contributor

The completeValue function is fairly large and took me a while to understand. This PR extracts out the logic that is different based on the type of the value being completed. Four new functions are added:

  • completeListValue
  • completeLeafValue
  • completeObjectValue
  • completeAbstractValue

The code in each function is identical except i inlined one isAbstractType call to avoid having to add an invariant.

I did add an invariant for an unreachable condition that also exists in the current logic in master, but ends on a return null.

This is a change that I did as part of #304. I'm submitting it as a separate PR. I think its a good change regardless of whether #304 is accepted.

Note that the names in this PR do not match #304. I tried to match the spec more closely with the names here and I will change the other to comply.

The exception to this is using the term leaf to refer to Scalar or Enum, which is established in type/definition.js, but not defined in the spec.

@leebyron
Copy link
Contributor

Thanks so much for the clear process! I'll dig into this.

@leebyron
Copy link
Contributor

Solid work, Jeff! A nice side effect of this that we can now reap is removing the flow type coercions that were within this block of code.

Also, since this reference implementation is designed to mirror the spec - perhaps we should make a similar refactoring to the relevant part of the spec http://facebook.github.io/graphql/#CompleteValue()

@leebyron leebyron merged commit 371a5a0 into graphql:master Mar 23, 2016
leebyron added a commit that referenced this pull request Mar 23, 2016
leebyron added a commit that referenced this pull request Mar 23, 2016
@JeffRMoore JeffRMoore deleted the split-complete-value branch March 24, 2016 06:26
@JeffRMoore
Copy link
Contributor Author

Thanks. I think it would help to split step 5 into step 5 and step 6, one for object, one for abstract type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants