Updated typings for database methods

[rhodgkins]

Fixes #6071. I've also update the return type for the onComplete handler that the methods also accept.

I've copied the comments for the TransactionResult interface from the corresponding @firebase/database package:

firebase-js-sdk/packages/database/src/api/Transaction.ts

Lines 42 to 49 in 14ccbad

	export class TransactionResult {
	/** @hideconstructor */
	constructor(
	/** Whether the transaction was successfully committed. */
	readonly committed: boolean,
	/** The resulting data snapshot. */
	readonly snapshot: DataSnapshot
	) {}

[changeset-bot]

🦋 Changeset detected

Latest commit: 8221684

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@firebase/database-types	Patch
@firebase/database-compat	Patch
firebase	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[hsubox76]

Looks good, in addition to the small nit about readonly (see comments), can you also add a changeset? To do this, run yarn changeset in the root of the repo and follow the prompts (select database-types as it suggests, then skip the "major" and "minor" options to let it default to "patch", then write a brief change note like "Updated typings for Reference methods"

[hsubox76]

The properties actually aren't readonly on the database-compat TransactionResult class, which is the only place this type will be applied, so it might be best to keep it looser and omit the readonly keyword:

firebase-js-sdk/packages/database-compat/src/api/TransactionResult.ts

Line 26 in b59a56d

constructor(public committed: boolean, public snapshot: DataSnapshot) {}

[hsubox76]

See above comment regarding readonly.

[rhodgkins]

@hsubox76 thanks for the review, readonlys removed and change set added 👍

[hsubox76]

It seems like the readonly has been removed from firebase/compat/index.d.ts but not from database-types/index.d.ts, is that the case or is it a Github refresh issue on my end?

Edit: oops, sorry, missed this earlier but the return types on the methods in database-compat are actually Promise<unknown>, not Promise<void>:

firebase-js-sdk/packages/database-compat/src/api/Reference.ts

Line 672 in 7343a55

remove(onComplete?: (a: Error | null) => void): Promise<unknown> {

Which you can see is causing problems in the CI tests. I actually think unknown is incorrect and your typings are correct, and database-compat/src/api/Reference.ts should be changed so the methods in conflict (update, remove, getPriority, etc) return Promise<void>. If this is getting to be too much of a headache for you, I can take this, but it's totally up to you.

[rhodgkins]

It seems like the readonly has been removed from firebase/compat/index.d.ts but not from database-types/index.d.ts, is that the case or is it a Github refresh issue on my end?

@hsubox76 ah sorry, totally missed that 🙈

database-compat/src/api/Reference.ts should be changed so the methods in conflict (update, remove, getPriority, etc) return Promise<void>

I'll have a go at changing those and see if that works :)

[rhodgkins]

@hsubox76 I've updated that file but there's still CI failures but its in a file I've not touched (I think anyway) - ERROR in /home/runner/work/firebase-js-sdk/firebase-js-sdk/packages/database/src/api/Database.ts, is something else needed?

[hsubox76]

@hsubox76 I've updated that file but there's still CI failures but its in a file I've not touched (I think anyway) - ERROR in /home/runner/work/firebase-js-sdk/firebase-js-sdk/packages/database/src/api/Database.ts, is something else needed?

It was a flaky test. I re-ran it and it passed. Everything looks good, will merge!

[rhodgkins]

Ah great, thanks 👍