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

Config option to make resolvers return values instead of pointers #2175

Merged

Conversation

ianling
Copy link
Contributor

@ianling ianling commented May 19, 2022

#1393

Adds a config option to make resolvers return values instead of pointers.

For example, given the following schema:

type User {
  id: ID!
  name: String!
}

type Query {
  user: User!
  maybeUser: User
}

Old behavior before this PR (matches default behavior after this PR)

func (r *queryResolver) User(ctx context.Context) (*User, error) {
	panic("not implemented")
}

func (r *queryResolver) MaybeUser(ctx context.Context) (*User, error) {
	panic("not implemented")
}

New behavior with resolvers_always_return_pointers: false (*User vs User in return type of first function):

func (r *queryResolver) User(ctx context.Context) (User, error) {
	panic("not implemented")
}

func (r *queryResolver) MaybeUser(ctx context.Context) (*User, error) {
	panic("not implemented")
}

I have:

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

@ianling
Copy link
Contributor Author

ianling commented May 19, 2022

Holding until my other PR is finished/merged

@coveralls
Copy link

coveralls commented May 20, 2022

Coverage Status

Coverage decreased (-0.05%) to 75.013% when pulling 770d6eb on ianling:resolvers_not_always_pointers into ddd825e on 99designs:master.

@ianling ianling force-pushed the resolvers_not_always_pointers branch from 4208a7b to e5baeaa Compare May 24, 2022 18:07
@nikitacrit
Copy link
Contributor

@ianling Thanks, I'm really looking forward to it. Any predictions when it will be merged?

@StevenACoffman
Copy link
Collaborator

StevenACoffman commented Jun 4, 2022

Sorry! I wanted to let the release sit for a few days for any urgent fixes for the recent WebSocket changes (since we don't use those in production at Khan Academy, so I always worry about breaking someone's edge case). I merged the other PR though.

@ianling ianling force-pushed the resolvers_not_always_pointers branch from e5baeaa to 770d6eb Compare June 6, 2022 19:49
@ianling ianling marked this pull request as ready for review June 6, 2022 19:49
@ianling
Copy link
Contributor Author

ianling commented Jun 6, 2022

Fixed merge conflicts.

Don't be afraid of the large change size, the actual change is only like 1 line, the rest is auto-generated models for tests.

And just to clarify, the default behavior of gqlgen remains the same after this PR is merged; users of gqlgen will only see different behavior if they add the new config option to their config.

@ianling
Copy link
Contributor Author

ianling commented Jun 6, 2022

Not sure why the Windows tests failed.

@StevenACoffman StevenACoffman merged commit 65e6810 into 99designs:master Jun 7, 2022
@ianling ianling deleted the resolvers_not_always_pointers branch June 7, 2022 16:29
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.

None yet

4 participants