search

zakodium / adonis-mongodb

MongoDB provider for AdonisJS 5
https://zakodium.github.io/adonis-mongodb/
Other
63 stars 12 forks source link

feat(Connection): add an observer api on transaction #160

Closed tpoisseau closed 5 months ago

tpoisseau commented 5 months ago

Refs: https://github.com/zakodium/profid/issues/1699

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.54%. Comparing base (7d4d94b) to head (d8ae4ab).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #160 +/- ## ========================================== + Coverage 92.37% 92.54% +0.16% ========================================== Files 9 10 +1 Lines 577 590 +13 Branches 72 78 +6 ========================================== + Hits 533 546 +13 Misses 44 44 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tpoisseau commented 5 months ago

It's tested but I'm not a big fan to rely on mongodb internals. If you found a better approach to known if transactions is aborted from public API I'm interested.

For testing I created a small helper promiseController to be able to resolve or reject outside the promise executor. I know it's quite unusual, I'm open to suggestion to avoid this pattern but keeping comprehensible testing code flow. Or use a similar built-in helper if exists.

targos commented 5 months ago

For testing I created a small helper promiseController to be able to resolve or reject outside the promise executor.

Maybe name it promiseWithResolvers or use a ponyfill of the standard API?

targos commented 5 months ago

If you found a better approach to known if transactions is aborted from public API I'm interested.

I remember already looking into it in the past and didn't find a public API

tpoisseau commented 5 months ago

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/withResolvers#browser_compatibility It's great we have this in ES api, It seems it supported since Node.js v22 so I'll not use it in this project. I'll rename so it match existing api and put a todo.