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

Abierto
  • ¿Algo no está bien?
#350 1 comentario 4 reacciones 0 asignados Ver en GitHub

Nadie ha tomado este issue todavía.

Evaluación

Dificultad
5/5
Tiempo estimado
Más de una semana
Aptitud para principiantes
25/100
Tipo de issue
Nueva funcionalidad
Claridad
Necesita aclaración
Estado de actividad
Estancado
Stack tecnológico
javascript, typescript
Área
api, backend

Línea de trabajo

Comienza leyendo los puntos de entrada load y loadMany y sus declaraciones de TypeScript; después, revisa el comportamiento documentado de las promesas y los errores descrito en este issue. Compara el enfoque propuesto basado en Result con los ejemplos existentes de gestión en V1 y V2; esto se hace únicamente una vez que el proyecto tenga un contrato de errores definido y coherente, junto con la documentación correspondiente.

Escrito por el modelo de indexación a partir del texto del issue.

Descripción

enhancement

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
Lenguaje dominante
JavaScript
Estrellas
13.4k
Forks
522
Sin PR fusionados en 30 d

Preparar el entorno

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de graphql/dataloader

Todos los issues de graphql/dataloader

Issues similares

Más issues de JavaScript

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.