dataloader-sequelize icon indicating copy to clipboard operation
dataloader-sequelize copied to clipboard

add support for basic findOne with ID

Open intellix opened this issue 10 months ago • 3 comments

Noticed that a few developers of our project were surprised that findOne didn't work until I swapped them to findByPk. Figured I'd have a go to see how much effort it was.

Next step would be adding support for cases like:

findOne({
  where: {
    id: 1,
    status: 'active'
  }
});

Which would probably cause a lot more of our codebase to use the dataloader.

  • Updated docker-compose file format (wasn't working for me locally without updating it)
  • Added support for basic findOne({ where: { id: 123 } }) usages.

For the tests, I copied findByPk and then made changes, and added an extra test that adding status: 'active' short circuits from the shim (until we add support for it).

intellix avatar Mar 25 '24 15:03 intellix

marking as draft for now until I fix the package-lock.json format and stop prettier running (and maybe tests pass).

Is the status: 'active' case easy? I feel like it's the same loader because the only thing that would change would be the ID

intellix avatar Mar 25 '24 15:03 intellix

marking as draft for now until I fix the package-lock.json format and stop prettier running (and maybe tests pass).

Is the status: 'active' case easy? I feel like it's the same loader because the only thing that would change would be the ID

Hm, I suppose it would not be too difficult to extract the primary key parameter and then try to determine what where clauses are equal.

mickhansen avatar Mar 26 '24 12:03 mickhansen

struggling to get the tests to pass for Sequelize 3 which has some weird prototyping stuff when it comes to spying on findAll (findOne calls Model.prototype.findAll). It's being called but the spy doesn't run. Everything passes in 4, 5, 6.

This is where the findAll is called, but it's not the spy instance: https://github.com/sequelize/sequelize/blob/v3.31.1/lib/model.js#L1527-L1530

versus how it works in v4 where it doesn't call it from prototype and it's the actual spy instance: https://github.com/sequelize/sequelize/blob/v4.44.4/lib/model.js#L1752-L1755

Do we need/want to support Sequelize 3? since it's more than 7 years old

intellix avatar Apr 07 '24 20:04 intellix