Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion spec/providers/database.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ describe('DeltaSnapshot', () => {
});
});

describe('#forEach(childAction: Function)', () => {
describe('#forEach(action: (a: DeltaSnapshot) => boolean): boolean', () => {
it('should iterate through child snapshots', () => {
populate({ a: 'b' }, { c: 'd' });
let out = '';
Expand All @@ -229,6 +229,32 @@ describe('DeltaSnapshot', () => {
subject.forEach(counter);
expect(count).to.eq(0);
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some checks here that the return values of both forEach() calls in this test are false?


it('should cancel further enumeration if callback returns true', () => {
populate(null, { a: 'b', c: 'd', e: 'f', g: 'h' });
let out = '';
const ret = subject.forEach(snap => {
if (snap.val() === 'f') {
return true;
}
out += snap.val();
});
expect(out).to.equal('bd');
expect(ret).to.equal(true);
});

it('should not cancel further enumeration if callback does not return true', () => {
populate(null, { a: 'b', c: 'd', e: 'f', g: 'h' });
let out = '';
const ret = subject.forEach(snap => {
if (snap.val() === 'a') {
return true;
}
out += snap.val();
});
expect(out).to.equal('bdfh');
expect(ret).to.equal(false);
});
});

describe('#numChildren()', () => {
Expand Down
3 changes: 1 addition & 2 deletions src/providers/database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,11 +206,10 @@ export class DeltaSnapshot implements firebase.database.DataSnapshot {
return valAt(this._delta, this._childPath) !== undefined;
}

// TODO(inlined) what is this boolean for?
forEach(action: (a: DeltaSnapshot) => boolean): boolean {
let val = this.val();
if (_.isPlainObject(val)) {
_.keys(val).forEach(key => action(this.child(key)));
return _.keys(val).some(key => action(this.child(key)));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lodash has a helper _.some() which we can use directly, instead of having to do _.keys(val).some().

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think we want to make sure this behavior only happens when true is returned, not when a truthy value (such as "a" or 1) is returned. So, this should probably end with action(this.child(key) === true. Can you please add a test for the case where action() returns 1 to ensure this behavior is correct?

}
return false;
}
Expand Down