[BREAKING] loadMany() returns individual Error instead of rejecting promise

[leebyron]

This changes the behavior of loadMore() to return Promise<Array<V | Error>> which always resolves, instead of the previous Promise<Array<V>> which rejects if any one of the requested values fails.

This makes this method meaningfully useful beyond a simple Promise.all()

[tuananh]

currently, if they don't want it to fail in Promise.all, they should have handle it in their load function?

we also have something new called Promise.allSettled that should work in this case?

[leebyron]

Batch loading functions should definitely return an Error at indices that failed when a batched result has a mix of success and failures.

You're totally right that this is essentially Promise.allSettled then mapping over the results to get values. However since this method is only supported in the most modern node/browser environments, we need to implement it manually here.

[statico]

I appreciate your hard work on this valuable library, so thank you. However, I think this change isn't for the better, and I'm compelled to express this even though it's been months since this was merged.

Our current loaders, used with TypeScript, GraphQL, and GraphQL Code Generator, are great. We can write resolvers like:

  products: async (parent, { filters }, { loaders }) => {
    const query = db().pluck('id').from('products')
    applyProductFilters(query, filters)
    const ids = await query
    return loaders.products.loadMany(ids)
  },

This is all fully typed, and TypeScript knows that the products resolver will return a Promise<Product[]>. However, with this change to loadMany, returning <Product | Error>[] won't make the resolver return type happy. The previous behavior of throwing an error if any object couldn't be loaded would be what I consider correct, since anything calling loadMany on our loaders should be dealing with consistent data. Now I'm not sure how to fix this other than casting loadMany(ids) as Product[], which is dirty, or doing some convoluted type guard with .filter() which seems even dirtier.

In short, I wish there had been a different method introduced instead of breaking the types of the current API in a way that doesn't seem to have a clear fix.

[Sarcasm]

a way that doesn't seem to have a clear fix.

The release notes mention a clear fix, no?

This will break any code which relied on .loadMany(). To support this change, either ensure the each item in the result of .loadMany() are checked against instanceof Error or replace calls like loader.loadMany([k1, k2]) with Promise.all([loader.load(k1), loader.load(k2)).
-- https://github.com/graphql/dataloader/releases/tag/v2.0.0

[statico]

Yes, the fix appears to be creating a subclass which implements the older behavior, but I wouldn't call that "clear."

class OldDataLoader<K, V, C = K> extends DataLoader<K, V, C> {
  async loadMany(keys: ArrayLike<K>): Promise<Array<V>> {
    const ret = await super.loadMany(keys)
    for (const v of ret) {
      if (v instanceof Error) {
        throw v
      }
    }
    return ret as Array<V>
  }
}

[Sarcasm]

Instead sticking to the old API, I would port the call sites, e.g. in you example:

   products: async (parent, { filters }, { loaders }) => {
     const query = db().pluck('id').from('products')
     applyProductFilters(query, filters)
     const ids = await query
-    return loaders.products.loadMany(ids)
+    return Promise.all(ids.map(id => loader.load(id)))
   },

[statico]

Thank you. I appreciate the help and suggestions.

[Cellule]

I understand that this has landed a long time ago now, but we just recently updated dataloaders on our project and stumbled on this breaking change.
We decided to go with the following .loadAll method instead in a subclass.

export class OurDataLoader<Key, Value, CacheKey> extends DataLoader<Key, Value, CacheKey> {
  /**
   * Loads multiple keys, promising an array of values:
   * Will error if any key rejects.
   * Reproduce behavior of dataloader@v1 loadMany
   */
  public async loadAll(keys: Key[]): Promise<Value[]> {
    return await Promise.all(keys.map((key) => this.load(key)));
  }
}

I chose to chime here mainly because I am curious if we know of real use cases for this new .loadMany behavior.
Why not simply make a new method?
Yes the old .loadMany was mainly just a "simple" wrapper, but I haven't found a good reason (in our project at least) to want separate errors.
Anyway almost all our dataloaders use batching and end up doing a single operation for the whole batch, so it's already an all or nothing kind of situation.

On the other hand, that new behavior is pretty much "just" a wrapper for Promise.allSettled(keys.map((key) => loader.load(key))).
So I don't agree with the initial argument that this change makes it "more meaningful".