feat: return numAffectedRows in Kysely driver

[WilcoKruijer]

Hi, thanks for the great project!

I moved a part of my project from better-sqlite3/Kysely to SQLocal/Kysely since I find it more convenient to work with a WASM blob rather than having to build a binary. In the process I noticed that I was no longer getting the numAffectedRows from Kysely's executeQuery call. This PR implement that.

To do this, I had to expose execSql method on the SQLocal/client class. Please let me know if this is not the way. It also adds a lastAffectedRows field to the transaction field.

Tests were added as well.

Cheers

[DallasHoff]

Thanks for the PR! This is looking good.

To do this, I had to expose execSql method on the SQLocal/client class. Please let me know if this is not the way.

Instead, I would add this type right below RawResultData in types.ts:

export type ResultsArray<Result extends Record<string, any>> = Result[] & {
	affectedRows?: bigint;
};

Then, modify the sql method:

sql = async <Result extends Record<string, any>>(
  queryTemplate: TemplateStringsArray | string,
  ...params: unknown[]
): Promise<ResultsArray<Result>> => {
  const statement = normalizeSql(queryTemplate, params);
  const { rows, columns, affectedRows } = await this.exec(
    statement.sql,
    statement.params,
    'all'
  );
  const results = convertRowsToObjects(rows, columns) as ResultsArray<Result>;
  results.affectedRows = affectedRows;
  return results;
};

[DallasHoff]

The docs hint that db.changes(false, true) can return a bigint. Did you test that?

[WilcoKruijer]

Good catch, I changed this to (false, true). Sadly the type on the changes method is wrong so I had to add an unsafe cast. I could also change the unsafe cast to re-construct the BigInt instead

[DallasHoff]

When sqlite-wasm adds new functions/signatures, they do not update the types unless someone submits a pull request. (The reason given is that the SQLite team does not know TypeScript and is not interested in maintaining types.) You should be able to get such a PR accepted easily enough though. I've done it before.

Until then, use a @ts-expect-error comment instead of a cast.

[DallasHoff]

I went ahead and opened that pull request to sqlite-wasm: sqlite/sqlite-wasm/pull/122

[WilcoKruijer]

Thanks for your review. I've addressed your concerns in a separate commit (I can squash later), see the other comments as well.

I removed the execSql method, and instead used the type you suggested:

export type ResultsArray<Result extends Record<string, any>> = Result[] & {
	affectedRows?: bigint;
};

Personally, I don't like this very much. Having extra properties on an array seems a bit magical to me. Also, a pretty big number of tests had to be changed as the following comparison would fail: expect(data).toEqual([{ name: 'x' }, { name: 'x' }]);, because data now contains an extra property. I've changed these to spread the data array: expect([...data]) ....

I think the even bigger concern is that this might break downstream tests (users of SQLocal). Lmk what you think

[DallasHoff]

My goal with that suggestion, besides facilitating the removal of execSql, was to expose affectedRows in a way that people can use if they want even if they don't use Kysely but without making it harder to read the data returned by sql. I considered the effect it would have on deep equality checks and figured since doing that is an anti-pattern anyway, it would be fine, but I forgot to consider the case of unit tests. Because of that, I agree that this approach isn't ideal either.

I think the better approach is to just un-private exec and return affectedRows in RawResultData. This will give users access to more info for advanced use-cases without changing the convenience of the sql method. As part of this change, beginTransaction should also return transactionKey so that it is usable with the exposed exec method.

[WilcoKruijer]

Okay, I used the changesBefore - changesAfter approach, and your other concerns should be addressed as well.

[DallasHoff]

Nothing major left. Just a couple little things. Thanks for working with me on this. I want to have this go in with version 0.17.

[DallasHoff]

Is it necessary to make a copy of the array here?

[DallasHoff]

We should declare const results = message.data[0] instead of duplicating it 3 times.

[DallasHoff]

Let's follow Kysely's lead and name the key numAffectedRows here too.

[DallasHoff]

Let's name this transactionKey.

[DallasHoff]

@ts-expect-error https://github.com/sqlite/sqlite-wasm/pull/122 will suffice for the comment.

[WilcoKruijer]

Okay, that should be all.

The array copy is necessary because the parameters array is readonly in Kysely. It would require a bunch of type changes throughout the code base to not need the copy. (Statement#parameters in types.ts could be made readonly)