Skip to content
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

add Result field to ResolverContext #301

Merged
merged 8 commits into from
Aug 24, 2018
Merged

add Result field to ResolverContext #301

merged 8 commits into from
Aug 24, 2018

Conversation

vvakame
Copy link
Collaborator

@vvakame vvakame commented Aug 21, 2018

@vvakame vvakame requested a review from vektah August 21, 2018 08:03
@vvakame vvakame changed the title add ParentObject & AncestorObject field to ResolverContext add ParentObject field to ResolverContext Aug 22, 2018
@vektah
Copy link
Collaborator

vektah commented Aug 22, 2018

Resolver context has a reference to the parent context as of #294.

type ResolverContext struct {
	Parent *ResolverContext
	// The name of the type this field belongs to
	Object string
	// These are the args after processing, they can be mutated in middleware to change what the resolver will get.
	Args map[string]interface{}
	// The raw field
	Field CollectedField

	// indicies tracks the array indicies only. all field aliases come from the context stack
	indicies []int
}

Perhaps this would be better represented by
graphql.GetResolverContext(ctx).Parent.Result
and would also allow for
graphql.GetResolverContext(ctx).Parent.Parent.Result

@vvakame
Copy link
Collaborator Author

vvakame commented Aug 22, 2018

this PR collide with #302 🤔

This PR introduce Result field to ResolverContext. and I refactor ResolverContext that looks like immutable.

@vvakame vvakame changed the title add ParentObject field to ResolverContext add Result field to ResolverContext Aug 22, 2018
rctx.PushIndex({{.index}})
defer rctx.Pop()
{{.arr}} = append({{.arr}}, func(ctx context.Context) graphql.Marshaler {
rctx := &graphql.ResolverContext{
Copy link
Collaborator

@vektah vektah Aug 23, 2018

Choose a reason for hiding this comment

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

this is strange. array marshalling isn't a resolver, and it shouldn't need a new context?

Copy link
Collaborator Author

@vvakame vvakame Aug 23, 2018

Choose a reason for hiding this comment

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

yes, it is right.
this change required default: case under case len(remainingMods) > 0 && remainingMods[0] == modList:.
I'll move this code to right place and add some arguments to method signature.

🤔 ❓

@@ -30,6 +30,8 @@ func (ec *executionContext) _{{$object.GQLType}}(ctx context.Context, sel ast.Se
ctx = graphql.WithResolverContext(ctx, &graphql.ResolverContext{
Object: {{$object.GQLType|quote}},
})
{{else}}
graphql.GetResolverContext(ctx).Result = obj
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably happen using res around field.gotpl:62, here its going to be repeated for every child

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah... make sense...
It was a bit different from what I thought.
so I'll write what I want to do.

@vvakame
Copy link
Collaborator Author

vvakame commented Aug 23, 2018

@vektah Result and ParentObject is not equals.
Resolver make []Todo, this is Result.
I think that Directive want a Todo, this is ParentObject.
so, I want to implement both. what do you think about this opinion?

@vvakame
Copy link
Collaborator Author

vvakame commented Aug 23, 2018

I noticed that there are multiple ideas of ResolverContext...

  1. Definition closely related to the current Resolver implementation.
  2. Every time Path changes, a new child ResolverContext is created.

I think the 2 is more convenient.
For example, if we want to issue an error in Resolver for some Index in Resolver that returns Array, we need to be able to generate new child ResolverContext ourself.
Specifically, if an illegal id is passed to nodes (ids: [ID!]!): [Node]!.

Also, when users browse / create ResolverContext, it is not known where asynchronous processing will be occurred.
For this reason, I think that ResolverContext created once, it should be immutable.
So, I think it is complicated to use PushIndex and Pop, and I think it is dangerous.

@vvakame
Copy link
Collaborator Author

vvakame commented Aug 23, 2018

It might be well refactored. 🤔

@vektah vektah mentioned this pull request Aug 24, 2018
2 tasks
@vektah
Copy link
Collaborator

vektah commented Aug 24, 2018

I still think its a bit weird to be creating a new resolver context for each result, currently all of that state stays on the stack as method args for normal resolvers. Perhaps this should work the same? see #314 for an alternative.

// indicies tracks the array indicies only. all field aliases come from the context stack
indicies []int
// The index of array in path.
Index *int
Copy link
Collaborator

Choose a reason for hiding this comment

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

then again I think this is a bit nicer.

@vektah vektah merged commit 9b24710 into master Aug 24, 2018
@vektah vektah deleted the feat-directive-parent branch August 24, 2018 04:01
cgxxv pushed a commit to cgxxv/gqlgen that referenced this pull request Mar 25, 2022
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