mickhansen / dataloader-sequelize

Batching and simplification of Sequelize with facebook/dataloader
MIT License
284 stars 58 forks source link

Can't get batching to work on latest Sequelize. #146

Closed Troplo closed 6 months ago

Troplo commented 11 months ago

Hi, I've been unable to get this module working correctly with Sequelize version 6.35.1 and Sequelize-TypeScript version 2.1.6 Upon looking at the query logs for MariaDB, running consecutive findByPk queries don't batch together and run separately.

let user1 = await User.findByPk(1, {
  [EXPECTED_OPTIONS_KEY]: ctx.dataloader
})
let user2 = await User.findByPk(6, {
  [EXPECTED_OPTIONS_KEY]: ctx.dataloader
})

Results in: image

Rather than IN (1,6) And I'm creating the dataloader object in my context generation function for every GraphQL operation. It appears the functions are being shimmed, as console.logging User.findByPk results in [Function: batchedFindById] which would imply the function is being routed through the module.

I noticed this repository hasn't been updated since 2021 so it appears it's no longer being maintained, but I thought I'd leave this issue here anyway.

mickhansen commented 11 months ago

Hi @Troplo,

It's not actively updated as it worked fine for the previous versions. It's odd that findByPk stopped working as I believe we shim at a pretty high level there.

I'll try to take a look when I'm able - and PRs are very welcome :)

Troplo commented 11 months ago

Thanks for the swift response, I had a look myself and I'm not entirely sure how it works, I see it does redirect it to loader.load(ID) separately, so 2 function calls for ID 1 and 6, it just doesn't query both of them together at the same time in a final query since it runs them separately, but other than that it definitely appears to be "working" as in it's definitely all being shimmed into Sequelize from the looks of it.

Maybe I'm completely misunderstanding what the project does, I'm honestly not sure.

mickhansen commented 11 months ago

Assuming the calls to .load() happen in parallel (within the same event loop tick), it should be batching them.

Troplo commented 11 months ago

Oh in that case it would be working fine then, if I put them both in a Promise.resolve it batches them, await would imply it of course wouldn't be in the same tick since it's waiting for the response of the last one, so I feel a bit stupid now.

Although the documentation here did make me think I could await both and it would still batch, so yeah actually appears to be working fine after all. img

In the context of a GraphQL field resolver, I assume I wouldn't await then, for example the following:

@FieldResolver(() => ChatInvite)
async invite(@Root() assoc: ChatAssociation, @Ctx() ctx: Context) {
  return await assoc.$get("invite", { [EXPECTED_OPTIONS_KEY]: ctx.dataloader })
}

would be instead this for the dataloader to work correctly and batch:

@FieldResolver(() => ChatInvite)
invite(@Root() assoc: ChatAssociation, @Ctx() ctx: Context) {
  return assoc.$get("invite", { [EXPECTED_OPTIONS_KEY]: ctx.dataloader })
}