Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rtdb: Reference#transaction return type #6071

Closed
rhodgkins opened this issue Mar 16, 2022 · 8 comments · Fixed by #6090
Closed

rtdb: Reference#transaction return type #6071

rhodgkins opened this issue Mar 16, 2022 · 8 comments · Fixed by #6090

Comments

@rhodgkins
Copy link
Contributor

[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 }>.

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
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

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).

): Promise<TransactionResult> {

@rhodgkins
Copy link
Contributor Author

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
Copy link
Contributor

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

transactionUpdate: (a: any) => any,

@rhodgkins
Copy link
Contributor Author

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:
image

@hsubox76
Copy link
Contributor

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
Copy link
Contributor Author

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.

@firebase firebase locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants