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

[REQUEST] Update types to support an array of promises as the resolved value for batchLoadFn #247

Open
Tracked by #297
chasingmaxwell opened this issue Aug 3, 2020 · 0 comments · May be fixed by #248
Open
Tracked by #297

Comments

@chasingmaxwell
Copy link

What problem are you trying to solve?

The DataLoader class already functionally handles the case where you resolve with an array of promises rather than an array of values. This is nice because it allows individual items in the batch to resolve separately and prevents the case where one long-running item in the batch holds up the rest of the items which were resolved quickly.

The problem is that the type definition for the batchLoadFn does not allow for individual items in the resolved array to be promises containing V. They must either be V or Error.

Here's an example.

const dataloader = new DataLoader<TheKey, TheValue>(
  // This will cause a flow error because TheValue does not equal Promise<TheValue>.
  async (keys: $ReadOnlyArray<TheKey>): Promise<Array<Promise<TheValue>>> => {
    return keys.map(key =>
      doSomethingTimeConsuming(key)
        .then(doMoreTimeConsumingThings)
        // Ensure errors are cached.
        .catch(e => e)
    );
  }
);

Describe the solution you'd like

The type BatchLoadFn could be updated to return Promise<$ReadOnlyArray<V | Promise<V> | Error>>.

Describe alternatives you've considered

I could just specify Promise<TheValue> as the return value for the dataloader, but then I have to do silly things like this to keep flow happy:

const theValue = await await dataloader.load(theKey);

Additional context

I'm working on a pull request now to address this if the maintainers decide this is a good change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant