Skip to content
This repository has been archived by the owner on Dec 19, 2019. It is now read-only.

Allow scalars as resolver return type #152

Closed
paliarush opened this issue Aug 20, 2018 · 1 comment
Closed

Allow scalars as resolver return type #152

paliarush opened this issue Aug 20, 2018 · 1 comment

Comments

@paliarush
Copy link
Contributor

paliarush commented Aug 20, 2018

Problem statement
All resolvers in Magento must implement Resolver interface, which in its turn forces the instance of \GraphQL\Deferred to be returned by each resolver. To be precise, Magento wrapper for deferred, \Magento\Framework\GraphQl\Query\Resolver\Value, must be returned.

/**
* Fetches the data from persistence models and format it according to the GraphQL schema.
*
* @param \Magento\Framework\GraphQl\Config\Element\Field $field
* @param $context
* @param ResolveInfo $info
* @param array|null $value
* @param array|null $args
* @throws \Exception
* @return Value
*/
public function resolve(
   Field $field,
   $context,
   ResolveInfo $info,
   array $value = null,
   array $args = null
) : Value;

The main reason for using deferred in GraphQL is to optimize performance by solving N+1 problem where it exists.
An example of valid deferred usage in Magento can be found in \Magento\CatalogGraphQl\Model\Resolver\Product::resolve.
In all other cases, when there is no need to solve N+1 problem, we end up with boilerplate code like:

$result = function () use ($data) {
    return $data;
};
 return $this->valueFactory->create($result);

Response type unification is one of the reasons why deferred was made the only possible return type for resolvers. However, after implementing more resolvers it became obvious that current design just adds unnecessary complexity to the resolver implementations.
Proposed solution
In addition to deferred, allow scalars and arrays of scalars as return types for \Magento\CatalogGraphQl\Model\Resolver\Product::resolve.
It will also be easier to customize resolver with plugins, if it returns array/scalar instead of deferred object.
Action items

  1. Modify existing resolvers, which have boilerplate code and do not require solving N+1 problem
    Change
 public function resolve(
     Field $field,
     $context,
     ResolveInfo $info,
     array $value = null,
     array $args = null
 ) : Value;

to

 public function resolve(
     Field $field,
     $context,
     ResolveInfo $info,
     array $value = null,
     array $args = null
 );

Also, replace boilerplate code in resolvers

$result = function () use ($data) {
    return $data;
};

return $this->valueFactory->create($result);

with

return $data;
  1. Document the decision and make sure that all new resolvers return deferred only when necessary. Can just mark PR with "requires-documentation" label to leave this part for Magento's documentation team.
@naydav
Copy link
Contributor

naydav commented Sep 12, 2018

#179

@naydav naydav closed this as completed Sep 12, 2018
magento-engcom-team added a commit that referenced this issue Nov 22, 2018


 - Merge Pull Request magento-engcom/import-export-improvements#152 from VoronoyAlexandr/import-export-improvements:125_Clean_Up_Magento/ImportExport/Block/Adminhtml/Export/Filter
 - Merged commits:
   1. 71e6f40
   2. 1f1ec54
naydav pushed a commit that referenced this issue Jun 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants