DataLoader.loadMany returns a rejected promise when an Error is loaded

[BartKrol]

I consider a new Error as a perfectly valid result when calling a load function so I can use them later in the chain. However when calling loadMany method I'm getting a rejected promise, which makes it impossible to use loaded data.

Here's an example test showing how to reproduce the issue:
https://gist.github.com/BartKrol/ad019183186c27edc0bb22617a71925d

[BartKrol]

Any update on that?

[alexandrebodin]

Hi.

Although that's a good point. We shouldn't change that behaviour as it would braek a lot of code using loadMany expecting to be able to catch after an error.

As stated in the doc loadMany is just a utility fonction and it serves as a dumb shortcut and It would be very easy for you to handle this case yourself.

Looking at loadMany implementation:

return Promise.all(keys.map(key => this.load(key)));

You can just do your own version of it like so

function myLoadMany(keys) {
  return Promise.all(
    keys.map(key => loader.load(key).catch(err => new Error(err))
  );
}

Hope it helped

[leebyron]

Hey @BartKrol - I think this is an interesting idea, but it's true that it would be a breaking change to the API.

Perhaps it was a mistake to include loadMany from the beginning since it was such a small convenience wrapper on Promise.all - I agree that it's perhaps more interesting if it provides behavior different from the more typical Promise.all usage.

Another challenge to consider is how to detect the difference between an Error object and a value - right now the library is using instanceof Error - but this is well known to be fragile if your environment spans JS realms. Not always a real problem, but can be in edge cases, right now an edge case not exposed.

[BartKrol]

I think we are talking about two separate problems here.

First one is a problem with loadMany method, which, as you explained, is just a wrapper around Promise.all. I guess you are right and this shouldn't be changed.

The second problem is something different. I consider a new Error in a load method as a perfectly valid response - after all I'm returning an error and not throwing it. Here's a code example:

function createByIdLoader(loadFunc) {
  return new DataLoader(
    ids => Promise.all(
      ids.map(loadFunc)
    ),
    {
      cache: false
    }
  )
}

describe('DataLoader', () => {
  it('should return an error', () => {  // Failing
    const value = 'fail'
    const stub = sinon.stub().returns(Promise.resolve(new Error(value)))
    const loader = createByIdLoader(stub)

    return loader.load(1).then(error => {
      expect(error).to.deep.equal(new Error(value))
    })
    .catch((err) => {
      console.log('Should never be here')
      throw err
    })
  })
})

[leebyron]

Sorry for letting this issue sit, I'll be closing it since it's starting to age.

Unfortunately DataLoader needs to use the Error returning behavior to determine mixed results from a Batch. Throwing an error would indicate that the entire batch request failed, however in the case that the batch request itself succeeded but some of the values within it failed, then an appropriate result is to return the list of resolved values where some values at indexes are Error instances instead of values.