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 dataloader pattern #159

Merged
merged 37 commits into from
Aug 14, 2020
Merged

Add dataloader pattern #159

merged 37 commits into from
Aug 14, 2020

Conversation

mike-marcacci
Copy link
Member

@mike-marcacci mike-marcacci commented Mar 15, 2020

This addresses #158 by using the dataloader pattern to batch and memoize entity calls within the same request.

The design followed here introduces the concept of an "execution context" or Executor. An Executor instance is unique to the request, and is to be used as a key to WeakMap caches of entities.

Special care must be taken to avoid reusing stale data after a mutation. Accordingly, the Executor instance is replaced in the broader context object by each mutation. Because GraphQL guarantees that mutations are run serially, we don't have to worry about race conditions here.

@mike-marcacci mike-marcacci marked this pull request as ready for review August 14, 2020 18:36
@mike-marcacci mike-marcacci requested a review from ebrown32 August 14, 2020 18:37

if (!a) {
throw new ForbiddenError(
"You must be authenticated to update an authority."
);
}

const strategies = executor.strategies;
const pool = executor.connection;
if (!(pool instanceof Pool)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

No idea if this is a good idea, but it seems like this code fragment appears an awful lot. Could we have some kind of executor.pool getter?

Copy link
Contributor

@ebrown32 ebrown32 left a comment

Choose a reason for hiding this comment

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

Just one comment, and it doesn't really matter that much.


if (!a) {
throw new ForbiddenError("You must be authenticated to create a client.");
}

const strategies = executor.strategies;
const pool = executor.connection;
if (!(pool instanceof Pool)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we do this a lot, we could change this to be some kind of executor.pool getter.

@mike-marcacci mike-marcacci merged commit f2d5988 into master Aug 14, 2020
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.

2 participants