Implement support for returning started transaction without using transaction() methods callback function #3099
Conversation
|
I'm not sure if it is confusing that meaning of a Promise that .transaction() returns is different based on provided params. It might be, and it may be a better solution to have separate method for this case, but I don't know what a good name that would be less confusing in that case would be. |
|
@kibertoad this looks useful. It will surely come in handy on our project at @LeapLabs. Thanks! |
|
@capaj You are welcome :) |
There was a problem hiding this comment.
Couple of minor comments. I was surprised that getting transaction was non-async method, meaning, that at least it haven't had time to acquireConnection and initialize it before it is returned.
Also I would like to see how errors are propagated in this case if connection cannot be acquired because all connections from pool are in use. That is easy to test by calling knex.transaction() multiple times.
|
Also might be a good idea to add to documentation something about availability of trx.initPromise for hooking functionality to transaction commit / rollback. |
…s://github.com/tgriesser/knex into feature/3095-direct-transaction
…s://github.com/tgriesser/knex into feature/3095-direct-transaction # Conflicts: # test/unit/knex.js # types/knex.d.ts
|
@elhigu Great point! There was, indeed, a gap in how errors were handled during transaction acquisition. |
|
Not really. initPromise is only used, as name implies, to track process of transaction itself getting prepared for usage. After this promise is resolved, it returns the transaction itself, and that's it. I don't think we should consider it a part of public api, as users are only supposed to consume it indirectly via knex.transaction() |
Right. I thought that promise was the promise that get resolved / rejected when trx.commit() / trx.rollback(err) was called. Like the one that is returned when knex.transaction(trx => { ... }) is called. |
|
@elhigu Hmmm, I can't find your other comment about your expectation for this not to start the actual transaction until someone actually uses it. I think that is doable, although would require slightly more intrusive changes. Do you think we should go for it within the scope of this PR? |
I don't think that is in the scope of this PR. I'll just rename the issue to be less confusing :) |
elhigu
changed the title
|
@elhigu It was a fairly trivial addition, though, so I ended up including it as a feature on top :) |
|
@elhigu Phew, build finally passes again. Could you please rereview? |
…tgriesser/knex into feature/3095-direct-transaction
…s://github.com/tgriesser/knex into feature/3095-direct-transaction # Conflicts: # types/index.d.ts
|
@capaj It was released as knex@0.17.0-next6, would be awesome if you could give it a try! Most likely final 0.17.0 stable will be published in a couple of days. |
| @@ -310,6 +310,40 @@ describe('knex', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| it('propagates error correctly when all connections are in use', function() { | |||
| this.timeout(30000); | |||
There was a problem hiding this comment.
Why this needs 30secs timeout? 🤔
There was a problem hiding this comment.
It is unlikely to use any more than just a couple of seconds, it is going to crash and be handled way earlier than that.
| var poolSqlite = { | ||
| min: 0, | ||
| max: 1, | ||
| acquireTimeoutMillis: 5000, |
There was a problem hiding this comment.
If possible we could try to have subsecond timeouts in tests to prevent test runs to take too long just waiting around.
There was a problem hiding this comment.
Tried reducing it, but I think I was getting some flaky tests that way. Decreased it, let's see how it goes.
Original inspiration behind this change was discussion on implementing DDD pattern in Javascript that we had at our company. Currently there is no convenient way for implementation-level (repositories) entities to generate a transaction instance that could then be passed from domain-level (use cases) entities across all the business logic execution steps without knowing what exact db operations are going to be executed.
fixes #3095