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.

Sign up for GitHub

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

Jump to bottom

rtdb(types): DataSnapshot#forEach - action parameter has null key #6368

Closed
rhodgkins opened this issue Jun 17, 2022 · 9 comments · Fixed by #6374
Closed

rtdb(types): DataSnapshot#forEach - action parameter has null key #6368

rhodgkins opened this issue Jun 17, 2022 · 9 comments · Fixed by #6374
Labels
api: database bug needs-attention v9

Comments

@rhodgkins
Copy link
Contributor

[REQUIRED] Describe your environment

  • Operating System version: N/A
  • Browser version: N/A
  • Firebase SDK version: 9.8
  • Firebase Product: database

[REQUIRED] Describe the problem

When using DataSnapshot#forEach the parameter available to the action closure is a DataSnapshot which is correct, but the key property of this value has the type string | null, but as its a child of the iterating snapshot it can't be the root node so its key can never be null.

Steps to reproduce:

const node = await getDatabase().ref().get()
node.forEach(child => {
   const key = child.key
   // key is possibly null so have to cast etc.
})

Relevant Code:

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

Line 25 in efe2000

forEach(action: (a: DataSnapshot) => boolean | void): boolean;

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

Lines 5821 to 5823 in efe2000

forEach(
action: (a: firebase.database.DataSnapshot) => boolean | void
): boolean;

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

Line 167 in efe2000

forEach(action: (snapshot: DataSnapshot) => boolean | void): boolean {

Fix

Happy to do a PR for this - I just remember it being a bit complicated last time I did this. Is the info in that issue still correct - the function signature in the above 3 places just needs to be changed?

Would changing the function signature to forEach(action: (a: DataSnapshot & { key: string }) => boolean | void): boolean; be OK for a fix?
@jbalidiong jbalidiong added the v9 label Jun 17, 2022
@maneesht
Copy link
Contributor

That syntax doesn't quite match v9, but your suggestions should work. For v9, you still need to update

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

Line 397 in d87d3a8

forEach(action: (child: DataSnapshot) => boolean | void): boolean {

@maneesht
Copy link
Contributor

If you could create a PR addressing this, that would be great

@rhodgkins
Copy link
Contributor Author

That syntax doesn't quite match v9

@maneesht Can you suggest some syntax that would match please!

If you could create a PR addressing this, that would be great

Will do!

@maneesht
Copy link
Contributor

If you're looking at the root, then:

import { getDatabase } from 'firebase/database';
const rootSnapshot = await get(ref(getDatabase()));
rootSnapshot.forEach(node => {
  console.log(node.key);
});

Out of curiosity, are you using v9-compat? (The non-modular version of firebase database)

@rhodgkins
Copy link
Contributor Author

rhodgkins commented Jun 21, 2022 •
edited
Loading

Oh I see the syntax of the example. Yes I'm using firebase-admin.

@hsubox76
Copy link
Contributor

Those 3 places you pointed out are the correct places to change, and although you are not using the v9 API, the compat API actually wraps the v9 API so you may have to change the line @maneesht indicated as well, because the compat forEach calls the v9 forEach here

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

Line 171 in efe2000

action(new DataSnapshot(this._database, expDataSnapshot))
and the signatures may have to match.

@rhodgkins
Copy link
Contributor Author

Great - thanks both.

@rhodgkins rhodgkins mentioned this issue Jun 22, 2022
Merged
@google-oss-bot google-oss-bot mentioned this issue Aug 4, 2022
Merged
@maneesht
Copy link
Contributor

@rhodgkins thank you for your contribution. However, we will have to rollback this change as it is causing a compilation error with AngularFire. We will add this back on the next major release. Until then, I will reopen this for tracking purposes.

@maneesht maneesht reopened this Aug 16, 2022
@maneesht maneesht mentioned this issue Aug 17, 2022
Merged
@maneesht
Copy link
Contributor

Closing this as it was part of the 10.0 release: https://firebase.google.com/support/release-notes/js#realtime-database

@firebase firebase locked and limited conversation to collaborators Sep 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
api: database bug needs-attention v9
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Updated type of action parameter for DataSnapshot#forEach bookcreator/firebase-js-sdk
6 participants
@rhodgkins @maneesht @DellaBitta @hsubox76 @google-oss-bot @jbalidiong