firebase-js-sdk icon indicating copy to clipboard operation
firebase-js-sdk copied to clipboard

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

Open rhodgkins opened this issue 3 years ago • 8 comments

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

https://github.com/firebase/firebase-js-sdk/blob/efe2000fc499e2c85c4e5e0fef6741ff3bad2eb0/packages/database-types/index.d.ts#L25

https://github.com/firebase/firebase-js-sdk/blob/efe2000fc499e2c85c4e5e0fef6741ff3bad2eb0/packages/firebase/compat/index.d.ts#L5821-L5823

https://github.com/firebase/firebase-js-sdk/blob/efe2000fc499e2c85c4e5e0fef6741ff3bad2eb0/packages/database-compat/src/api/Reference.ts#L167

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?

rhodgkins avatar Jun 17 '22 14:06 rhodgkins

That syntax doesn't quite match v9, but your suggestions should work. For v9, you still need to update https://github.com/firebase/firebase-js-sdk/blob/d87d3a8b8cd68e757be6628a72538bfd303e78d1/packages/database/src/api/Reference_impl.ts#L397

maneesht avatar Jun 21 '22 16:06 maneesht

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

maneesht avatar Jun 21 '22 16:06 maneesht

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!

rhodgkins avatar Jun 21 '22 17:06 rhodgkins

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)

maneesht avatar Jun 21 '22 17:06 maneesht

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

rhodgkins avatar Jun 21 '22 17:06 rhodgkins

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 https://github.com/firebase/firebase-js-sdk/blob/efe2000fc499e2c85c4e5e0fef6741ff3bad2eb0/packages/database-compat/src/api/Reference.ts#L171 and the signatures may have to match.

hsubox76 avatar Jun 21 '22 17:06 hsubox76

Great - thanks both.

rhodgkins avatar Jun 21 '22 17:06 rhodgkins

@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 avatar Aug 16 '22 04:08 maneesht

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

maneesht avatar Aug 17 '23 19:08 maneesht