Skip to content

Consistently name the first resolver argument and the root value#505

Merged
vladar merged 9 commits intowebonyx:masterfrom
spawnia:root-value-consistent-naming
Jul 3, 2019
Merged

Consistently name the first resolver argument and the root value#505
vladar merged 9 commits intowebonyx:masterfrom
spawnia:root-value-consistent-naming

Conversation

@spawnia
Copy link
Collaborator

@spawnia spawnia commented Jun 23, 2019

Unified all references to the root value to mixed $rootValue for consistency.

The first resolver argument is now called $objectValue when the type is not known. For root field resolvers, it is called $rootValue, for other fields it is named after the Object Type the fields are defined on.

This is a non-breaking, trivial refactoring, as only argument names are changed.

@vladar
Copy link
Member

vladar commented Jul 1, 2019

Looks like there is some misunderstanding here. $rootValue is only used on the very top-level in the GraphQL::executeQuery() call.

Let me try to explain. In general GraphQL deals with types and values. Resolver of each type field receives a value of this type. Only Query type resolvers receive $rootValue. In a sense $rootValue is a value of the type Query.

So the correct naming for the first resolver argument is either just $value or a type-related name like $fooValue or $foo (for a type Foo).

# Conflicts:
#	docs/data-fetching.md
#	src/Executor/Executor.php
#	tests/Executor/DeferredFieldsTest.php
@spawnia
Copy link
Collaborator Author

spawnia commented Jul 1, 2019

@vladar i get your point.

Let's make it so that rootValue is only and always used when the actual top-level root is referenced.

In nested resolvers, it would be optimal to have a speaking name for the argument, as you mentioned with $foo for a type Foo. However, we do have parts of the code and the docs that contain generic resolvers where such a type is not known.

I looked around the spec and reference implementation, they seem to use obj or objectValue.

I think parent would be acceptable as well. Right now, there is a mix between $source, $v, $val or $value.

My personal vote goes towards $obj since it is consistent with the reference implementation.

@vladar
Copy link
Member

vladar commented Jul 1, 2019

The problem I have with $obj is that it is a bit misleading. The value doesn't have to be an object (or even associative array). It can be any scalar/resource as well (even if it seems weird but technically that's the case). $objectValue is the most accurate description but it's too long to type.

So I would suggest using $objectValue for generic/default resolvers as well as in docs (to make the intent clear). But for type-specific resolvers - it should be allowed to name the argument after type name ($foo or $fooValue) or just $value. I guess we could replace $val and $v with $value for consistency.

As for $parent - I always had a problem with it because a field is a part of a type, but not a child. Parent-child relationship usually refers to values of the same kind which is not the case with fields and types. It's like calling a person's hand it's child.

Unfortunately the reference implementation uses this naming in ResolveInfo (parentType property) so we had to follow there, but the argument still applies.

Anyways if anyone else has their opinion - please let us know.

@spawnia
Copy link
Collaborator Author

spawnia commented Jul 1, 2019

Yeah, let's go with $objectValue. Maybe we can rename $info to $resolveInfo as well, as we don't match the reference anyways at that point and prefer more explicit naming.

@spawnia spawnia changed the title Consistently name the $rootValue argument Consistently name the first resolver argument Jul 1, 2019
@spawnia spawnia changed the title Consistently name the first resolver argument Consistently name the first resolver argument and the root value Jul 1, 2019
@vladar vladar merged commit 22a0da9 into webonyx:master Jul 3, 2019
@spawnia spawnia deleted the root-value-consistent-naming branch July 17, 2019 18:48
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.

4 participants