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

Plugin: ForceResolver #354

Closed
wants to merge 11 commits into from
Closed

Plugin: ForceResolver #354

wants to merge 11 commits into from

Conversation

mathewbyrne
Copy link
Contributor

This is a first attempt to chip away at progressing a plugin system from #228. This implements a single plugin resolver that adds a directive @resolver to the schema, and allows a user to tag fields in their schema as requiring a resolver; this is the same functionality exposed in config.

Plugins

After a bit of discussion in #228 and elsewhere, our initial direction for supporting plugins will be to ship the gqlgen binary with a set of base plugins, and let users generate their own binary if they want to add additional plugins themselves.

We considered some other directions, such as using the plugin package — but have decided to keep it simple while we're actually developing the capabilities of the system. Feedback is welcome here as to how this will fit into your existing workflows.

Registration

Plugins are registered via codegen.RegisterPlugin and should satisfy codegen.Plugin. It is currently expected that this would be done in a plugin package's init function.

Hooks

This PR adds 2 ways that plugins can hook into codegen functionality:

  1. Plugin.Schema should return an *ast.Source (perhaps should be []*ast.Source?). These will be merged with the user's schema during parsing, allowing a plugin to add to a schema as it sees fit. Since this occurs before parsing, it may be too limiting for some plugin use-cases. We're investigating whether we need to separate out the parsing and validation steps in the parser.

  2. Plugin.Execute is called after config normalization, and allows a plugin to arbitrarily modify configuration prior to codegen.

These two hooks are sufficient for the resolver plugin to function, but we definitely understand that there will be lots of other use-cases that require additional hooks. My plan with this PR is to validate this general approach, and understand how we could expand (or even limit) it towards additional plugin use-cases.


func (p *Plugin) Schema(cfg *codegen.Config) (*ast.Source, error) {
return &ast.Source{
Name: "resolver plugin",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this should be done by the caller and this function can just return the schema string?

cfg.Directives["resolver"] = codegen.DirectiveMapEntry{Implementation: "github.com/99designs/gqlgen/codegen/plugins/resolver.DirectiveNoop"}
for _, typ := range schema.Types {
for _, f := range typ.Fields {
if d := f.Directives.ForName("resolver"); d != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if d == nil { continue }?

codegen/plugins/resolver/plugin.go Outdated Show resolved Hide resolved
"github.com/vektah/gqlparser/ast"
)

type Plugin interface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want one large interface with lots of methods or lots of small interfaces that can optionally be implemented?

eg we may eventually have codegen hooks here, but most plugins wont use them.

Copy link
Contributor

Choose a reason for hiding this comment

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

lots of 1-function interfaces will be easier to use, then one big.

codegen/codegen.go Outdated Show resolved Hide resolved
@vetcher
Copy link
Contributor

vetcher commented Sep 25, 2018

As I understand, in this realization, I need to rebuild gen every time I add new plugin?

@vektah
Copy link
Collaborator

vektah commented Sep 25, 2018

@vetcher theres a few different fronts that can be advanced here but its tricky. Right now, we are focusing on what the plugins can do rather then how they get injected.

We've thought a little about using go plugins (and probably will support it in the future) but it adds a bunch of complexity:

For now we are just going to side step that complexity by building a bunch of plugins into the binary, and making it easy to build your own entry point.

fun fact, gorunpkg github.com/99designs/gqlgen is already building gen on demand from source.

@mathewbyrne mathewbyrne mentioned this pull request Sep 25, 2018
@vetcher
Copy link
Contributor

vetcher commented Oct 4, 2018

@vektah you may add a line to config, name it plugins and, on start up, just open list of files and do nothing with them. Something like that.

func initPlugins(plugins []string) error {
	for i := range plugins {
		_, err := plugin.Open(plugins[i])
		if err != nil {
			return errors.Wrapf(err, "open plugin %s", plugins[i])
		}
	}
	return nil
}

And in plugin we just need to add 3 lines:

func init() {
	codegen.RegisterPlugin(plugin)
}

It can be nice addition to current realisation and user does not need to recompile gqlgen each time.

Mathew Byrne added 5 commits October 4, 2018 20:40
Improve grep-ability, since it took me a few mins to find with other
usages of Resolver.
Take @vektah's suggestions, I've removed the global plugin registry and
looked at implementing separate interfaces for each hook that a plugin
might want to support.
@mathewbyrne
Copy link
Contributor Author

@vetcher this PR is really more about exposing functionality for plugins to hook into. We definitely will consider plugin distribution carefully once we have a better idea how they will integrate. Feel free to move this sort of discussion into #228

@mathewbyrne
Copy link
Contributor Author

@vektah ok I've had another pass over this now. Key changes:

  1. Multiple interfaces for a plugin to define what functionality they want to support.
  2. Changed plugin schema interface to just return a string.
  3. Updated parsedSchema in generated output to add plugin schemas (removed SchemaRaw)
  4. Inject plugins through config in gen and init commands.

@@ -9,26 +9,26 @@ import (
"time"
)

type resolver struct {
type chat struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe resolver is too generic a plugin name; this clashed so I changed the resolver struct name here.

@mathewbyrne
Copy link
Contributor Author

For context — I've closed this for now. We're thinking about doing a plugin release the one after next and focussing on it entirely. This has been around long enough now though that our thinking has shifted a little and we might approach this slightly differently.

@porty porty deleted the plugin-forced-resolver branch February 8, 2019 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants