From a0300e020aa541b8622937374122a5ad8c16a8a6 Mon Sep 17 00:00:00 2001 From: Jiahui Ruan Date: Wed, 18 Mar 2020 15:48:13 -0700 Subject: [PATCH 1/4] handle case when requestGroups return single error object --- src/runtimeHelpers.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/runtimeHelpers.ts b/src/runtimeHelpers.ts index f4fcb0c..6038bf7 100644 --- a/src/runtimeHelpers.ts +++ b/src/runtimeHelpers.ts @@ -201,7 +201,7 @@ export function unPartitionResults( /** Should be a nested array of IDs, as generated by partitionItems */ requestGroups: ReadonlyArray>, /** The results back from the service, in the same shape as groups */ - resultGroups: ReadonlyArray>, + resultGroups: ReadonlyArray>, ): ReadonlyArray { /** * e.g. with our inputs, produce: @@ -217,7 +217,13 @@ export function unPartitionResults( * ] * ``` */ - const zippedGroups = requestGroups.map((ids, i) => ids.map((id, j) => ({ order: id, result: resultGroups[i][j] }))); + const zippedGroups = requestGroups.map((ids, i) => + ids.map((id, j) => ({ + order: id, + // @ts-ignore: in some cases, resultGroups[i] would be an Error rather than an array + result: resultGroups[i] instanceof Error ? resultGroups[i] : resultGroups[i][j], + })), + ); /** * Flatten and sort the groups - e.g.: From 4b404e5c069de083874e1a61a6925d12e5fff645 Mon Sep 17 00:00:00 2001 From: Jiahui Ruan Date: Wed, 18 Mar 2020 16:42:57 -0700 Subject: [PATCH 2/4] add tests for unPartitionResults --- __tests__/runtimeHelpers.test.js | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 __tests__/runtimeHelpers.test.js diff --git a/__tests__/runtimeHelpers.test.js b/__tests__/runtimeHelpers.test.js new file mode 100644 index 0000000..f7a0867 --- /dev/null +++ b/__tests__/runtimeHelpers.test.js @@ -0,0 +1,29 @@ +import { unPartitionResults } from '../src/runtimeHelpers'; + +describe('unPartitionResults', () => { + it('should perform inverse mapping for result without Error', () => { + expect(unPartitionResults([[0, 2], [1]], [[{ foo: 'foo' }, { bar: 'bar' }], [{ baz: 'baz' }]])).toEqual([ + { foo: 'foo' }, + { baz: 'baz' }, + { bar: 'bar' }, + ]); + }); + + it('should perform inverse mapping for result with Error in resultGroups', () => { + const customError = new Error('bar error'); + expect(unPartitionResults([[0, 2], [1]], [[{ foo: 'foo' }, customError], [{ baz: 'baz' }]])).toEqual([ + { foo: 'foo' }, + { baz: 'baz' }, + customError, + ]); + }); + + it('should perform inverse mapping for result with Error as an item in resultGroups', () => { + const customError = new Error('foo error'); + expect(unPartitionResults([[0, 2], [1]], [customError, [{ baz: 'baz' }]])).toEqual([ + customError, + { baz: 'baz' }, + customError, + ]); + }); +}); From 67d1583d955988f33bb7df6a57c16ab7cd6b03d8 Mon Sep 17 00:00:00 2001 From: Jiahui Ruan Date: Wed, 18 Mar 2020 18:47:40 -0700 Subject: [PATCH 3/4] use invariant to tell flow that it's an Error --- __tests__/runtimeHelpers.test.js | 6 +++--- src/implementation.ts | 4 ++++ src/runtimeHelpers.ts | 10 ++-------- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/__tests__/runtimeHelpers.test.js b/__tests__/runtimeHelpers.test.js index f7a0867..83c3eaf 100644 --- a/__tests__/runtimeHelpers.test.js +++ b/__tests__/runtimeHelpers.test.js @@ -9,7 +9,7 @@ describe('unPartitionResults', () => { ]); }); - it('should perform inverse mapping for result with Error in resultGroups', () => { + it('should perform inverse mapping for result with some Error in resultGroups', () => { const customError = new Error('bar error'); expect(unPartitionResults([[0, 2], [1]], [[{ foo: 'foo' }, customError], [{ baz: 'baz' }]])).toEqual([ { foo: 'foo' }, @@ -18,9 +18,9 @@ describe('unPartitionResults', () => { ]); }); - it('should perform inverse mapping for result with Error as an item in resultGroups', () => { + it('should perform inverse mapping for result with all Error in one resultGroup', () => { const customError = new Error('foo error'); - expect(unPartitionResults([[0, 2], [1]], [customError, [{ baz: 'baz' }]])).toEqual([ + expect(unPartitionResults([[0, 2], [1]], [[customError, customError], [{ baz: 'baz' }]])).toEqual([ customError, { baz: 'baz' }, customError, diff --git a/src/implementation.ts b/src/implementation.ts index 70c10b0..f78a6a9 100644 --- a/src/implementation.ts +++ b/src/implementation.ts @@ -249,6 +249,10 @@ function getBatchLoader(resourceConfig: BatchResourceConfig, resourcePath: Reado )} Resource returned \${response.length} items, but we requested \${requests.length} items.\`, 'Add reorderResultsByKey to the config for this resource to be able to handle a partial response.', ].join(' ')); + + // Tell flow that "response" is actually an error object. + // (This is so we can pass it as 'cause' to CaughtResourceError) + invariant(response instanceof Error, 'expected BatchItemNotFoundError to be an Error'); } } `; diff --git a/src/runtimeHelpers.ts b/src/runtimeHelpers.ts index 6038bf7..f4fcb0c 100644 --- a/src/runtimeHelpers.ts +++ b/src/runtimeHelpers.ts @@ -201,7 +201,7 @@ export function unPartitionResults( /** Should be a nested array of IDs, as generated by partitionItems */ requestGroups: ReadonlyArray>, /** The results back from the service, in the same shape as groups */ - resultGroups: ReadonlyArray>, + resultGroups: ReadonlyArray>, ): ReadonlyArray { /** * e.g. with our inputs, produce: @@ -217,13 +217,7 @@ export function unPartitionResults( * ] * ``` */ - const zippedGroups = requestGroups.map((ids, i) => - ids.map((id, j) => ({ - order: id, - // @ts-ignore: in some cases, resultGroups[i] would be an Error rather than an array - result: resultGroups[i] instanceof Error ? resultGroups[i] : resultGroups[i][j], - })), - ); + const zippedGroups = requestGroups.map((ids, i) => ids.map((id, j) => ({ order: id, result: resultGroups[i][j] }))); /** * Flatten and sort the groups - e.g.: From b26f633e6f397e33dc998c1238bf7355a68d0b18 Mon Sep 17 00:00:00 2001 From: Jiahui Ruan Date: Thu, 19 Mar 2020 10:33:01 -0700 Subject: [PATCH 4/4] add comments to explain why we add invariant for BatchItemNotFoundError --- src/implementation.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/implementation.ts b/src/implementation.ts index f78a6a9..e184f85 100644 --- a/src/implementation.ts +++ b/src/implementation.ts @@ -250,8 +250,9 @@ function getBatchLoader(resourceConfig: BatchResourceConfig, resourcePath: Reado 'Add reorderResultsByKey to the config for this resource to be able to handle a partial response.', ].join(' ')); - // Tell flow that "response" is actually an error object. - // (This is so we can pass it as 'cause' to CaughtResourceError) + // Tell flow that BatchItemNotFoundError extends Error. + // It's an issue with flowgen package, but not an issue with Flow. + // @see https://github.com/Yelp/dataloader-codegen/pull/35#discussion_r394777533 invariant(response instanceof Error, 'expected BatchItemNotFoundError to be an Error'); } }