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

Graphql batch resolver #293

Merged
merged 11 commits into from
Oct 25, 2019
Merged

Conversation

AlexMaxHorkun
Copy link
Contributor

Problem

Current ResolverInterface does provide tools nor steer developers into creation of fast batch resolvers. By introducing new BatchResolverInterface developers will receive an array of values and arguments to resolve data for in optimal never without requiring them to mess with callbacks
and duplicated values and arguments gathering and promises management logic.

Solution

Introduce BatchResolverInterface, manage promises and create batches of values and arguments on
framework level. Deprecate existing interface

Requested Reviewers

@paliarush

@AlexMaxHorkun AlexMaxHorkun mentioned this pull request Sep 25, 2019
@paliarush paliarush self-assigned this Sep 25, 2019
@AlexMaxHorkun
Copy link
Contributor Author

I've added batch resolvers that contact a service contract via RESTful API to demonstrate batch resolving for distributed setup

Copy link
Contributor

@paliarush paliarush left a comment

Choose a reason for hiding this comment

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

I suggest to promote this approach as advanced functionality. After some time we will be able to evaluate acceptance rate and make further strategy adjustments.

Please do not make Batch resolver a requirement and do not enforce its usage via static tests.

/**
* @return array|null
*/
public function getValue(): ?array;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add comprehensive description to the getters.

design-documents/graph-ql/batch-resolver.md Outdated Show resolved Hide resolved
/**
* Resolve multiple brunches/leaves by executing a batch service contract.
*/
interface BatchContractResolverInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the following change makes sense

Suggested change
interface BatchContractResolverInterface
interface BatchServiceContractResolverInterface

* @return object
* @throws GraphQlInputException
*/
public function convertToArgument(ResolveRequestInterface $request);
Copy link
Contributor

Choose a reason for hiding this comment

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

How about

Suggested change
public function convertToArgument(ResolveRequestInterface $request);
public function prepareRequest(ResolveRequestInterface $request);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't usually use request with regard to class/service contract arguments. prepareArgument perhaps?

* @return ResolveInfo
*/
public function getInfo(): ResolveInfo;

/**
* Data passed from upper resolvers.
Copy link
Contributor

Choose a reason for hiding this comment

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

From the parent resolver

@akaplya akaplya merged commit 86bd5ff into magento:master Oct 25, 2019
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.

3 participants