-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Correct resolver batching behavior #1495
Changes from 24 commits
0409de1
64933a7
d4414df
61634d2
853ddfe
e15e609
f3e17b7
cfed7e8
1588751
3253bbe
cc2e485
a7fb234
bf62cca
81be746
f87db3a
ce1530d
3b298fd
b063d48
a9db97b
aeba69f
028ee26
9d1507f
5e807a1
15ee783
8c55da8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,11 +58,11 @@ describe("batchLoader", () => { | |
) | ||
const batch = batchLoader({ multipleLoader }) | ||
|
||
await batch({ id: "foo", param: "bar" }) | ||
await batch({ id: ["foo"], param: "bar" }) | ||
expect(multipleLoader).toBeCalledWith({ | ||
batched: true, | ||
id: ["foo"], | ||
param: "bar", | ||
size: 1, | ||
}) | ||
}) | ||
|
||
|
@@ -79,55 +79,58 @@ describe("batchLoader", () => { | |
|
||
expect(multipleLoader).toBeCalledTimes(1) | ||
}) | ||
}) | ||
}) | ||
|
||
it("should return a default value when an id is missing", async () => { | ||
const multipleLoader = jest.fn(({ id: ids }) => | ||
ids.slice(1).map(id => ({ | ||
_id: id, | ||
})) | ||
) | ||
const batch = batchLoader({ multipleLoader, defaultResult: {} }) | ||
|
||
const results = await Promise.all([ | ||
batch("123"), | ||
batch("456"), | ||
batch("789"), | ||
]) | ||
expect(results).toEqual([ | ||
{}, | ||
{ | ||
_id: "456", | ||
}, | ||
{ | ||
_id: "789", | ||
}, | ||
]) | ||
describe("serializeParams", () => { | ||
const { serializeParams } = require("../batchLoader") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Btw, why are you loading these functions this way? If you would import them as normal, you could do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good tip! I'm doing the imports this way because earlier in the tests I mock the dependency to override the feature flag. https:/artsy/metaphysics/blob/batching-bugfix/src/lib/loaders/__tests__/batchLoader.test.ts#L10 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, but actually it doesn’t seem like you’d need to do any mocking for that, you can just change the value and reset it from an Anyways, not a blocker for this PR. |
||
|
||
expect(multipleLoader).toBeCalledWith({ | ||
id: ["123", "456", "789"], | ||
size: 3, | ||
}) | ||
}) | ||
it("should return an empty string for a param object with only id", () => { | ||
expect(serializeParams({ id: "a" })).toBe("") | ||
}) | ||
it("should return key=value for every entry in the params object", () => { | ||
expect(serializeParams({ id: "a", foo: "bar", bleep: "bloop" })).toBe( | ||
"bleep=bloop&foo=bar" | ||
) | ||
}) | ||
}) | ||
|
||
const { groupKeys } = require("../batchLoader") | ||
describe("groupKeys", () => { | ||
describe("groupByParams", () => { | ||
const { groupByParams } = require("../batchLoader") | ||
it("should handle a single key", () => { | ||
expect(groupKeys(["a"])).toEqual([ | ||
{ | ||
id: ["a"], | ||
size: 1, | ||
}, | ||
expect(groupByParams([{ id: "a" }])).toEqual([ | ||
[""], | ||
alloy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
[ | ||
[ | ||
{ | ||
id: "a", | ||
}, | ||
], | ||
], | ||
]) | ||
}) | ||
|
||
it("should group single keys together", () => { | ||
expect(groupKeys(["a", "b", "c"])).toEqual([ | ||
const keys = [ | ||
{ | ||
id: ["a", "b", "c"], | ||
size: 3, | ||
id: "a", | ||
}, | ||
{ | ||
id: "b", | ||
}, | ||
] | ||
expect(groupByParams(keys)).toEqual([ | ||
[""], | ||
[ | ||
[ | ||
{ | ||
id: "a", | ||
}, | ||
{ | ||
id: "b", | ||
}, | ||
], | ||
], | ||
]) | ||
}) | ||
|
||
|
@@ -142,38 +145,75 @@ describe("groupKeys", () => { | |
foo: "bar", | ||
}, | ||
] | ||
expect(groupKeys(keys)).toEqual([ | ||
{ | ||
id: ["a", "b"], | ||
foo: "bar", | ||
size: 2, | ||
}, | ||
expect(groupByParams(keys)).toEqual([ | ||
["foo=bar"], | ||
[ | ||
[ | ||
{ | ||
id: "a", | ||
foo: "bar", | ||
}, | ||
|
||
{ | ||
id: "b", | ||
foo: "bar", | ||
}, | ||
], | ||
], | ||
]) | ||
}) | ||
|
||
it("should separate keys with different parameters", () => { | ||
const keys = [ | ||
"a", | ||
{ id: "a" }, | ||
{ id: "b", foo: "bar" }, | ||
{ id: "c", foo: "bar", boo: "baz" }, | ||
] | ||
|
||
expect(groupKeys(keys)).toEqual([ | ||
{ | ||
id: ["a"], | ||
size: 1, | ||
}, | ||
{ | ||
id: ["b"], | ||
foo: "bar", | ||
size: 1, | ||
}, | ||
{ | ||
id: ["c"], | ||
foo: "bar", | ||
boo: "baz", | ||
size: 1, | ||
}, | ||
expect(groupByParams(keys)).toEqual([ | ||
["", "foo=bar", "boo=baz&foo=bar"], | ||
[ | ||
[ | ||
{ | ||
id: "a", | ||
}, | ||
], | ||
[ | ||
{ | ||
id: "b", | ||
foo: "bar", | ||
}, | ||
], | ||
[ | ||
{ | ||
id: "c", | ||
foo: "bar", | ||
boo: "baz", | ||
}, | ||
], | ||
], | ||
]) | ||
}) | ||
}) | ||
|
||
describe("cacheKeyFn", () => { | ||
const { cacheKeyFn } = require("../batchLoader") | ||
zephraph marked this conversation as resolved.
Show resolved
Hide resolved
|
||
it("should not treat two objects with the same id but different params as different", () => { | ||
expect(cacheKeyFn({ id: "123" })).not.toEqual( | ||
cacheKeyFn({ id: "123", foo: "bar" }) | ||
) | ||
}) | ||
|
||
it("should treat two objects with the same params and id as equal", () => { | ||
expect(cacheKeyFn({ id: "123", foo: "bar" })).toEqual( | ||
cacheKeyFn({ id: "123", foo: "bar" }) | ||
) | ||
}) | ||
|
||
it("should treat two objects with different ids as different", () => { | ||
expect(cacheKeyFn({ id: "456", foo: "bar" })).not.toEqual({ | ||
id: "567", | ||
foo: "bar", | ||
}) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL. Fancy ✨