loadBy*With => loadBy*

[rkatic]

Our loaders are still not well adopted, and I think the main reason could be the API.

const loadByIdWith = loader.one({ from: 'user', by: 'id' })
//            ^^^^ WTF 

const user = await userRepo.loadByIdWith(db)(userId) // method hard to read
//                                  ^^^^^^^^ WTF

Motivation for such "strange" API was to make it less error prone, making it really hard to forget to pass db/t.

However passing t to functions is a larger issue that loaders alone are not resolving (for all other non-loader repo functions).

So, in an effort to come with an acceptable compromise that would make loaders being actually used, I first considered an API based on optional arguments.

const loadById = loader.one({ from: 'user', by: 'id' })
//    ^^^^^^^^ nice

const user = await userRepo.loadById(userId) // nice and simple

const user = await userRepo.loadById(userId, t) // kinda nice

const users = await asyncMap(userIds, userRepo.loadById) // throws, index passed as second argument
const users = await asyncMap(userIds, id => userRepo.loadById(id)) // now works

First issue is that it's easy to miss that we passed (or not) the t making it less maintainable and error-prone.

Also, loadById could be a custom function that is not accepting the second argument (ignoring it). TS would mitigate this issue, but that's out of scope of this PR.

New API

Instead of optional arguments, we can pass optional stuff with a .using(...) method.

const loadById = loader.one({ from: 'user', by: 'id' }) // COOL

const user = await userRepo.loadById(userId) // nice, simple

const user = await userRepo.loadById.using(t)(userId) // I see it and I know `t` will not be ignored by `loadById`

const user = await userRepo.loadById.using(t, 'FOR UPDATE')(userId) // long, but still readable

const users = await asyncMap(userIds, userRepo.loadById) // works
const users = await asyncMap(userIds, userRepo.loadById.using(t, 'FOR UPDATE')) // works

Note that if a loader doesn't supports transactions, it will miss .using(...) and fail, preventing many silent bugs. Similarly, if loader supports transactions but not locking, use of second argument will fail an internal check.

Updated Loader docs

Any thoughts?

[rkatic]

I'm giving a second though on all this .using() thing. Smells on replacing one strange API with another strange one. So considering the optional arguments approach again, by perhaps adding some external TS declaration files to cover at least loader.all/one.

[rkatic]

Although I'm still not 100% convinced we came with an optimal API (if there is one), I think we can go with this one. In case we decide later that the "optional arguments" approach is better, we will be able to adopt that one as well. Proceeding with merging.