Disallow transaction to return a Promise due to unexpected behaviour

[phiresky]

As described in #319:

You should never keep SQLite3 transactions open across event loop ticks in Node.js. SQLite3 serializes all statements/transactions. This means, if you try to run an asynchronous function within an SQL statement, you'll block all other statements/transactions across your entire program. better-sqlite3 doesn't support it, and it never will, because it's a very bad idea.

This is reasonable. But: Currently if a user passes an async function as a transaction callback, the resulting behaviour is very confusing and dangerous: It will run the first statement within the transaction, commit the transaction, and then the other part of the async function will run at a random time later without any transaction context, errors not being rolled back.

Instead, the least we can do is detect this case (that a lot of users have already hit if you look at the multiple issues opened about this) and reject it.

Edit for people searching: The code now throws TypeError: Transaction function cannot return a promise

[mceachen]

FWIW I think this is a reasonable PR that may save someone a couple minutes (or hours) of debugging. Thanks!

[michaelts1]

@JoshuaWise, After upgrading from better-sqlite3 11.2, my previously working code now errors due to this change. Shouldn't this be listed as a "breaking change" in the release notes?
(ideally such a change should also be done in a major version but I guess it's way too late for that)

[phiresky]

It is a breaking change kind of yes. It's very unlikely though that the code you have actually does what you want unless your transaction has a single statement or no awaits. Could you send your code or an equivalent? @michaelts1

[michaelts1]

To be clear, I am not opposed to this change at all. I was not aware of the behavior explained in the pull request, and learning about it, I agree that throwing an error is the correct approach.
In my case there was no actual need for the transaction itself to be async, so this was not a problem and I was able to fix it easily.

I am pointing this out since I was surprised when my code threw an error after a minor upgrade that had no "breaking change" listed. I can imagine this being a problem for an application that defines an async transaction only in some code paths, where the developer might not immediately realize that the upgrade will require modifying the code.

[phiresky]

Yes, I agree with you. I see there's a lot of PRs referencing this PR that made changes because of this, which is both good and bad. The cleanest option would probably to revert in another minor version and then publish a major version. But it's probably too late and you could kind of argue this PR only changes the behaviour of a code path that previously was not supported in any case, so it's not breaking.

Also, it would probably good to implement this in the typescript types https://github.com/DefinitelyTyped/DefinitelyTyped/blob/d5dcfe8db3c38034efa60d74df3e87800e43e507/types/better-sqlite3/index.d.ts#L36 so it throws at compile times as well (if someone has time).