[REQUEST] Unify the way `load` and `loadMany` handle errors

[james-yeoman]

What problem are you trying to solve?

At the moment, the way that load and loadMany are handled is very different.

load is a regular promise that rejects, so awaits need to be try-catched.

loadMany always resolves, so we need to iterate over the results in order to determine if we couldn't get anything.

Since there's no indication in the Typescript declaration that one always resolves and the other doesn't. Not even a JSDoc to mention that if the batch function returns an error, the promise will reject.

Describe the solution you'd like

Unification in how we need to handle errors. Maybe something like Rust's Result type could be useful. Neverthrow looks like a robust implementation of Result

Describe alternatives you've considered

• Sticking with V1. Not as ideal, since the errors are useful for providing additional context to errors in a centralised location. Our custom error messages no longer need to include the entity ID.

• Tried several different ways of handling the errors, as detailed in the below additional context. We're struggling to come to a consensus on it, due to the fact that we need to be cognisant of the fact that load may throw if an ID isn't found in our DB.

Additional context

I'm having some trouble on my team trying to migrate to Dataloader v2. The improved error information is great, but migrating from V1 just feels unergonomic.

Places where we would check for null are now utter chaos.

We've tried try-catch blocks but the sheer amount of boilerplate that it introduces (can't use const x = loader.load(...) anymore) is unergonomic and verbose.

We've tried promises, which feels ok at the loader callsite, since Prettier wraps the line nicely. But the wrappers are a bit unsightly with wrapWithContextError being a curryied function.

await loader
	.loadMany(idArray)
	.then(throwIfError)
	.catch(wrapWithContextError(wrapperError))

The fact that loader.load needs to be handled significantly differently to loader.loadMany has brought with it some confusion.

Here's the load variants

// =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
// =-=-=- Allow the error to propagate =-=-=-=
// =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
const entity = await ctx.loaders.entityLoader.load(id);
// Do something with the entity

// =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
// =-=-=- Allow the error to propagate, but wrap in a custom error =-=-=-=
// =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
let entity;
try {
	entity = await ctx.loaders.entityLoader.load(id);
} catch (e) {
	throw new UserInputError(`Could not find <entity name>`, {
		cause: e
	});
}
// Do something with the entity

// Or

const entity = await ctx.loaders.entityLoader.load(id).catch(e => {
	throw new UserInputError(`Could not find <entity name>`, {
		cause: e
	});
});
// Do something with the entity

// =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
// =-=-=- Promises, ignoring the error =-=-=-=
// =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
const entity = await ctx.loaders.entityLoader.load(id).catch(() => null);
// Do something with the entity

Here's the loadMany variants

// =-=-=-=-=-=-=-=-=-=-=-=
// =-=-=- Promises =-=-=-=
// =-=-=-=-=-=-=-=-=-=-=-=
const entities = await ctx.loaders.entityLoader
	.loadMany(ids)
	.then(throwIfError());
// Do something with the collection

// =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
// =-=-=- Promises with a custom error =-=-=-=
// =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
const entities = await ctx.loaders.entityLoader
	.loadMany(ids)
	.then(throwIfError())
	.catch(wrapWithContextError(
		new UserInputError("One or more <entity name>s could not be found")
	));
// Do something with the collection

// =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
// =-=-=- Promises, ignoring the error =-=-=-=
// =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
const entities = await ctx.loaders.entityLoader
	.loadMany(ids)
	.then(entities => entities.map(
		entity => isError(entity) ? null : entity)
	);
// Do something with the collection


// =-=-=-=-=-=-=-=-=-=-=-=-=
// =-=-=- Type guard =-=-=-=
// =-=-=-=-=-=-=-=-=-=-=-=-=
const entities = await ctx.loaders.entityLoader.loadMany(ids);

if (!loadSuccessful(entities)) {
	throw entities.find(isError);
}
// Do something with the collection

[loucadufault]

What about just having .load() also always resolve, to an Error or value of type V. This would unify the API:

.load() always resolves to -> Error | V
.loadMany() always resolves to -> Array<Error | V>