-
Notifications
You must be signed in to change notification settings - Fork 205
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
Fixed issue calling DataSnapshot methods with null data #1661
base: master
Are you sure you want to change the base?
Conversation
@taeold IIUC this is a straight bugfix, but I wanted to double check that you don't consider this a breaking change. |
Feels like this is a good change and not breaking. IIUC the change would only break if the traversing the full path instead of short-circuiting somehow had a side effect that was worthy. |
@taeold any ideas on the timeline when this might be merged / released? |
4a917c5
to
29ff57d
Compare
Sorry I missed that there were tests in the project for this! I've added some tests for the issue to make it clearer. |
@@ -128,7 +128,7 @@ export class DataSnapshot implements database.DataSnapshot { | |||
let source = this._data; | |||
if (parts.length) { | |||
for (const part of parts) { | |||
if (source[part] === undefined) { | |||
if (typeof source === "undefined" || source === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As well as moving this "up" to the parent object I've also changed the check from === undefined
to a typeof
so it matches the other checks elsewhere:
if (typeof val === "undefined" || val === null) { |
if (node === null || typeof node === "undefined") { |
Description
Fixes firebase/firebase-functions-test#254
Fix is similar to the check in
#exists()
:firebase-functions/src/common/providers/database.ts
Line 173 in 5f7b331