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

Đang mở
  • Có gì không ổn?
#350 1 bình luận 4 reaction 0 người được giao Xem trên GitHub

Chưa có ai nhận issue này.

Đánh giá

Độ khó
5/5
Thời gian dự kiến
Hơn một tuần
Mức phù hợp với người mới
25/100
Loại issue
Tính năng
Độ rõ ràng
Cần làm rõ
Mức độ hoạt động
Đình trệ
Công nghệ
javascript, typescript
Lĩnh vực
api, backend

Hướng nghiên cứu

Bắt đầu bằng cách đọc các entry point load và loadMany cùng các khai báo TypeScript của chúng, sau đó xem lại hành vi promise và lỗi đã được tài liệu hóa trong issue này. So sánh cách tiếp cận theo kiểu Result được đề xuất với các ví dụ xử lý hiện có trong V1 và V2; việc này chỉ được thực hiện sau khi dự án đã xác định một hợp đồng lỗi nhất quán và tài liệu tương ứng.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Mô tả

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
Ngôn ngữ chính
JavaScript
Star
13.4k
Fork
522
Không có pull request nào được merge trong 30 ngày

Chuẩn bị môi trường

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Issue khác của graphql/dataloader

Tất cả issue của graphql/dataloader

Issue tương tự

Thêm issue về JavaScript

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.