Improve ResolverImplementer.Implment

[roneli]

Changes the ResolverImplementor to be a function that is called in render time, rather then before, keeping compatibility with previous versions.

@StevenACoffman ,I will add tests if this change makes sense.

The reason I changed it is sometimes the render requires some logic, such as calling the template.Render's ReserveImport , which impossible to call if we are doing it before that call as it will be nil (templates.CurrentImports)

Moreover the original implementation had a flaw, that once an implementation existed, it was never called again, which doesn't make sense. I wanted to pass the PrevDecl of the function to the interface, but didn't want to break compatibility, so now it just re-renders each time.

Describe your PR and link to any relevant issues.

I have:

• Added tests covering the bug / feature (see testing)
• Updated any relevant documentation (see docs)

[coveralls]

coverage: 75.933% (+0.08%) from 75.856%
when pulling 1bb2ce7 on roneli:improve-resolver-implement-render
into cb3c1c8 on 99designs:master.

[StevenACoffman]

Excellent! This is an exciting improvement, and is relevant to (at least) these several open issues:

• Generated Federation code for fields with @external directive applied on a list which is used in a @requires field is wrong. #2559
• gqlgen generates non compilable federation.go code when schema uses @requires directive with attribute referring inside array for dependency  #2632
• non-nullable @external field that are not part of @key fieldset blows up federation #2357
• Federation: requiring a field with a list of objects generates invalid code #1861
• Support for nested selection sets on _FieldSet (on @requires annotation) #1138

I had hoped that the @jesse-apollo and the Apollo team would have used a similar technique to retain backward compatibility in their former PR:

• WIP: Refactor federation requires to populate on entity via a function #2676

[StevenACoffman]

@roneli I would like you to add some test coverage for this, as I expect others to build upon your work here.

[roneli]

Awesome, thanks for the quick reply.

I wanted to also upgrade this code a little more, as currently it only allows a single implementer, but I would like implementers to have more control perhaps (running based on field / logic) and also get previous implementation. Maybe for future improvements. (Unless you prefer we break the interface now? rather than later?)

Will add tests soon so we can merge this.

[roneli]

Added a test, as I saw on all, I would maybe go with deeper tests, need to play with more cases like a render with imports.

Just tell me if we want to upgrade the interface a little more now to give it more control or in a later PR?

// ResolverImplementer is used to generate code inside resolvers
type ResolverImplementer interface {
	ShouldImplement(object *codegen.Object, field *codegen.Field) bool
	Implement(previousDecl string, field *codegen.Field) string
}

Like this perhaps? Allowing for implementors based on what we see maybe a directive etc'.

We can also just pass the object into implement and let all the logic reside there.

[StevenACoffman]

This PR is already a useful improvement, so I merged it! I would love further improvements or demonstrations of how to take advantage of this.

I am open to changing the interface in the future if we can demonstrate an immediate need to do so, and we have a good sense of how much it might break people's existing code.

[mojtabacazi]

This breaks our entire project, replaces are existing resolver implementation with panic(....

[StevenACoffman]

@mojtabacazi Did you look at #2886 ? WDYT?