rtdb: Reference#transaction return type

[rhodgkins]

[REQUIRED] Describe your environment

• Operating System version: macOS
• Browser version: N/A
• Firebase SDK version: firebase-admin@10
• Firebase Product: database

[REQUIRED] Describe the problem

The transaction method on a reference is typed to return Promise<any> as opposed to Promise<{ committed: boolean, snapshot: DataSnapshot }>.

firebase-js-sdk/packages/database-types/index.d.ts

Lines 140 to 144 in a7f4a2e

	transaction(
	transactionUpdate: (a: any) => any,
	onComplete?: (a: Error | null, b: boolean, c: DataSnapshot | null) => any,
	applyLocally?: boolean
	): Promise<any>;

This is the same problem in the docs.

I'm happy to create a PR to update the above file, but not sure if that's the correct place to do this?

Steps to reproduce:

const { getDatabase } = require('firebase-admin/database')

const result = await getDatabase().ref('blah').transaction(data => data)

Result is any

Relevant Code:

See above.

[jbalidiong]

Hi @rhodgkins, from looking at your issue, I believe you should file an issue with the firebase-admin repo: https://github.com/firebase/firebase-admin-node

[rhodgkins]

@jbalidiong

Are you sure - when looking at the definitions for firebase-admin, they reference @firebase/database-types which is the file above?

Anyway, can't you migrate this issue over (I've seen that happen for admin issues that get moved here)?

[hsubox76]

Reopened, as we do own @firebase/database-types. Sorry about that. What issues is this typing causing? If we want to set more specific return types for the methods on Reference (which might be a good idea) I think the right way to do it would be to do all the methods together. It would be weird to change this one specifically while leaving all the other methods returning Promise<any>.

Specifically it looks like the database-types definitions could be updated to reflect the more specific types now used in database-compat source (in this case TransactionResult).

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

Line 694 in cdada6c

): Promise<TransactionResult> {

[rhodgkins]

Thanks @hsubox76, yeh the main issue is the lack of typing for autocompletion - I've had a few silly bugs with the the spelling of committed 😳
I'm happy to do a PR for the other methods as well if that helps, is it just a case of updating the file I originally referenced?

[hsubox76]

Can you double check that fixing that file in your local installation fixes it? Just manually change node_modules/@firebase/database-types/index.d.ts. This might be the right file if you're using it through firebase-admin. But for users of the client SDK I think the file that typings are pulled from for autocomplete is node_modules/firebase/compat/index.d.ts (a large umbrella file containing all the types for all packages) so to maintain consistency it should probably be changed to match in the same PR.

The source file for the second one would be

firebase-js-sdk/packages/firebase/compat/index.d.ts

Line 7278 in 4bd3a88

transactionUpdate: (a: any) => any,

[rhodgkins]

I've updated database-types/index.d.ts locally to import TransactionResult from @firebase/database and then corrected typed transaction (the onComplete parameter return type still needs changing but doesn't matter for this...):

  transaction(
    transactionUpdate: (a: any) => any,
    onComplete?: (a: Error | null, b: boolean, c: DataSnapshot | null) => any,
    applyLocally?: boolean
  ): Promise<TransactionResult>;

And now when using await getDatabase().ref('blah').transaction(data => data) the return types appear correctly, however there's some clashes now going on between firebase-admin's database.DataSnapshot and the one defined in a TransactionResult (from @firebase/database). Before I was getting around the lack of type in the Promise by using some JSDoc:

/** @type {{ committed: boolean, snapshot: import('firebase-admin/database').DataSnapshot }} */
const { committed, snapshot } = await getDatabase().ref('blah').transaction(data => data)

Which was working fine, but now after changing to TransactionResult, I'm now getting the following error when using snapshot:

Types of property 'snapshot' are incompatible.
    Type 'import("<snip>/node_modules/@firebase/database/dist/public").DataSnapshot' is not 
    assignable to type 'import("<snip>/node_modules/@firebase/database-types/index").DataSnapshot'.ts(2322)

Is there an easy way to work around this?

Just to add, I looked at the compat way but database-compat for me has the runTransaction defined:

[hsubox76]

Yes, database-compat is fine, that's the source code. What needs to be changed in tandem is packages/firebase/compat/index.d.ts. That's not used by admin but it is used by client-side SDK users who import from firebase/database-compat, so it would need to be updated simultaneously with database-types in any PR. If you want to make this change, TransactionResult should not be imported as a class from @firebase/database but defined from scratch in database-types/index.d.ts as an interface.

Basically, the database-types is a separate typings package necessary for the previous (non-modular) version of the Firebase SDK, which is now provided as -compat packages, while the default packages are all modular. firebase-admin currently needs to use the non-modular version until admin is able to update how it consumes the package, so it is using database-compat. In the old pattern, the -types packages need to stand alone as just types, no code, and are then imported as types by the -compat packages. In the new pattern, type definition (*.d.ts) files are generated directly from the source code, as is the case in database, and a separate -types file is not used.

[rhodgkins]

OK, I think I understand now - I'll have a go at a PR updating transaction, set, update and remove and see how it goes.