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 configurable namespace to allow custom non-root type field resolvers without FQN #1711

Closed
Ririshi opened this issue Jan 30, 2021 · 7 comments · Fixed by #2351
Closed
Labels
discussion Requires input from multiple people

Comments

@Ririshi
Copy link

Ririshi commented Jan 30, 2021

I originally wrote this issue differently, but rewrote it after realizing Lighthouse doesn't work the way I thought it did w.r.t. non-root type field resolvers

What problem does this feature proposal attempt to solve?

My team is working on a fairly large GraphQL API for a restaurant platform. This is our first project using GraphQL, which lead to a lot of "not following best practices" when we just started. We are now tidying up our GraphQL schema, and I found that a lot of our non-root type field resolvers are using a long @field directive with a fully qualified class name. I would like to see a method where non-root types can define custom field resolvers without a FQN in the @field directive.

Which possible solutions should be considered?

I came up with a rough idea that would tackle this problem.

  1. We should specify a fields (or types) namespace in lighthouse.php, just like the namespaces that already exist for queries, mutations, etc. This in itself would already greatly decrease clutter for custom field resolvers, as one could already point to resolver classes in this namespace without using their FQN when using the @field directive. For the sake of this example, let's say we defined fields as App\GraphQL\Fields.

  2. To make things a bit neater, I propsose we allow sub-namespaces in the (single) fields namespace. Each of these will represent a GraphQL type. Since GraphQL types are uniquely named, every sub-namespace would uniquely cover each type's fields, as long as we limit the namespace search to at most one level deeper than the originally specified fields namespace. In this case, one would use a structure like App\GraphQL\Fields\Restaurant\PhoneNumber to specify a resolver for the phoneNumber field of the Restaurant object. This resolver will only be used if the field specifies @field(resolver: "PhoneNumber").

  3. Finally, with this extra structure in place, we could automatically use custom field resolvers if one exists for a specific type field. Let's take the previous Restaurant and phoneNumber as an example. If the fields namespace has a sub-namespace called Restaurant, we search inside this for a PhoneNumber class, and automatically use this as the field resolver, in a similar fashion to how queries and mutations automatically pick up custom resolvers.

I found #1580 while looking if someone already suggested what I'd like to see. While my idea is different than the idea proposed by the author of this feature request, I would understand if the stance on namespaces would make this an unviable solution. I personally think namespaces are valuable rather than harmful, because different types can have fields with the same name (and namespaces allow exactly this behaviour). Even without steps 2 and 3, just implementing step 1 would already make our schema a lot neater (with shorter lines because we don't need to use FQNs).

@Ririshi Ririshi closed this as completed Jan 30, 2021
@Ririshi Ririshi changed the title Allow different type fields with same name to have different custom field resolvers, without requiring @field directive Add configurable namespace to allow custom non-root type field resolvers without FQN Jan 31, 2021
@Ririshi Ririshi reopened this Jan 31, 2021
@spawnia spawnia added the discussion Requires input from multiple people label Jan 31, 2021
@spawnia
Copy link
Collaborator

spawnia commented Jan 31, 2021

Regarding namespaces: We should use them where they line up with the structure of the GraphQL schema. Types fall within the root GraphQL namespace and thus should not be put into sub-namespaces. On the other hand, fields are naturally namespaced within their respective type, and we should also represent that within the PHP namespaces. Those are different scenarios.

I propose we add another root namespace in lighthouse.php:

    'namespaces' => [
        'models' => ['App', 'App\\Models'],
        'queries' => 'App\\GraphQL\\Queries',
        ...
+       'types' => 'App\\GraphQL\\Types',
        'interfaces' => 'App\\GraphQL\\Interfaces',
        ...
    ],

This brings parity with the namespaces for other root level type classes, such as interfaces. From this perspective, the root namespaces for queries, mutations and subscriptions are rather special cases and could just as well be nested under types. For historical reasons and/or convencience we might just leave them as is, but I am now considering changing them to this in the next major version:

    'namespaces' => [
        'queries' => 'App\\GraphQL\\Types\\Query',
    ],

With this new types namespace in place, we can now look for all field resolvers in the following order:

  • resolver directives, e.g. @field or @method
  • convention-based class, e.g. App\GraphQL\Queries\Restaurants, App\GraphQL\Types\Restaurant\PhoneNumber
  • default resolver for non-root fields, error for root fields

@lorado
Copy link
Collaborator

lorado commented Feb 1, 2021

Just a question - do accessors (like getPhoneNumberAttribute()) count to "default resolver"?
I hope the bug with double-calling will be fixed soon, anyway I don't want to lose this accessor-feature.

Apart from that - great idea. It would make lighthouse more flexible, but also more clearly structured due convention-based classes, like App\GraphQL\Types\Restaurant\PhoneNumber. It probably could make some people confused about that much possibilities to fetch data, but a good documentation should cover it. It would be also helpful to mention in the docs, which way at what situation should be used, like

  • @filed and @method should be used, if you plan to reuse the resolve logic
  • convention-based class syntax should be used, when you use a custom type, that is not an Eloquent model, or you want to override default eloquent resolver
  • write an accessor on eloquent model (to stay clean in code? IDK)

@spawnia
Copy link
Collaborator

spawnia commented Feb 1, 2021

Just a question - do accessors (like getPhoneNumberAttribute()) count to "default resolver"?

Yes, the default resolver calls them through simple property access on the $root.

I like your suggestions regarding the docs, that is some good advice to give.

@olivernybroe
Copy link
Collaborator

@lorado Just a note about cleaning your schema, utilizing @method and @rename directive takes care of a lot of field resolvers.
Using @find, @paginate, @builder, @eq, @whereAuth (recently added support for field types with some of theses) and so on, can often remove a lot of your query resolvers.

'queries' => 'App\GraphQL\Types\Query',

I think this looks nice. However we have to be careful that we are not just adding more namespaces with very little gain.
One of the biggest advantages I see with this, is that I think we can have some pretty simple logic so resolve all types.

@spawnia
Copy link
Collaborator

spawnia commented Feb 1, 2021

'queries' => 'App\GraphQL\Types\Query',

The next major version of Lighthouse could actually drop explicit namespaces for the root type fields, so that line would not even be in lighthouse.php anymore. The types namespace already covers that.

@lorado
Copy link
Collaborator

lorado commented Feb 1, 2021

@lorado Just a note about cleaning your schema, utilizing @method and @rename directive takes care of a lot of field resolvers.
Using @find, @paginate, @builder, @eq, @whereAuth (recently added support for field types with some of theses) and so on, can often remove a lot of your query resolvers.

Right, almost all my root queries/mutations are actually using those directives. But in other Types fields, especially Eloquent models, it feels to me better to use accessors, so I don't need to write an extra directive to resolve a field. Moreover I can use accessors in my regular laravel code, e.g. in emails generating or cron jobs.

@Ririshi
Copy link
Author

Ririshi commented Feb 1, 2021

Not exactly on topic of the discussion, but you should probably escape those @directive mentions when quoting someone, @lorado.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Requires input from multiple people
Projects
None yet
4 participants