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

Align DataSnapshot methods to @firebase/database DataSnapshot #926

Closed
wants to merge 3 commits into from
Closed

Align DataSnapshot methods to @firebase/database DataSnapshot #926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dfd1708
Test cases for DataSnapshot to align methods closer to @firebase/data…
rhodgkins Jul 31, 2021
92597f0
Aligned DataSnapshot methods to @firebase/database counterpart
rhodgkins Jul 31, 2021
f08be25
Removed unneeded use of _.isArray with _.isObjectLike
rhodgkins Aug 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
143 changes: 134 additions & 9 deletions spec/v1/providers/database.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,17 +541,52 @@ describe('Database Functions', () => {
expect(subject.val()).to.equal(0);
populate({ myKey: 0 });
expect(subject.val()).to.deep.equal({ myKey: 0 });

// Null values are still reported as null.
populate({ myKey: null });
expect(subject.val()).to.deep.equal({ myKey: null });
Comment on lines -544 to -547
Copy link
Contributor Author

@rhodgkins rhodgkins Jul 31, 2021 •
edited

Choose a reason for hiding this comment

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

I've removed this test case as its incorrect compared to the Firebase SDK, the correct version is now in should deal with null-values appropriately.

});

// Regression test: .val() was returning array of nulls when there's a property called length (BUG#37683995)
it('should return correct values when data has "length" property', () => {
populate({ length: 3, foo: 'bar' });
expect(subject.val()).to.deep.equal({ length: 3, foo: 'bar' });
});

it('should deal with null-values appropriately', () => {
populate(null);
expect(subject.val()).to.be.null;

populate({ myKey: null });
expect(subject.val()).to.be.null;
});

it('should deal with empty object values appropriately', () => {
populate({});
expect(subject.val()).to.be.null;

populate({ myKey: {} });
expect(subject.val()).to.be.null;

populate({ myKey: { child: null } });
expect(subject.val()).to.be.null;
});

it('should deal with empty array values appropriately', () => {
populate([]);
expect(subject.val()).to.be.null;

populate({ myKey: [] });
expect(subject.val()).to.be.null;

populate({ myKey: [null] });
expect(subject.val()).to.be.null;

populate({ myKey: [{}] });
expect(subject.val()).to.be.null;

populate({ myKey: [{ myKey: null }] });
expect(subject.val()).to.be.null;

populate({ myKey: [{ myKey: {} }] });
expect(subject.val()).to.be.null;
});
});

describe('#child(): DataSnapshot', () => {
Expand All @@ -578,14 +613,37 @@ describe('Database Functions', () => {
});

it('should be false for a non-existent value', () => {
populate({ a: { b: 'c' } });
populate({ a: { b: 'c', nullChild: null } });
expect(subject.child('d').exists()).to.be.false;
expect(subject.child('nullChild').exists()).to.be.false;
});

it('should be false for a value pathed beyond a leaf', () => {
populate({ a: { b: 'c' } });
expect(subject.child('a/b/c').exists()).to.be.false;
});

it('should be false for an empty object value', () => {
populate({ a: {} });
expect(subject.child('a').exists()).to.be.false;

populate({ a: { child: null } });
expect(subject.child('a').exists()).to.be.false;

populate({ a: { child: {} } });
expect(subject.child('a').exists()).to.be.false;
});

it('should be false for an empty array value', () => {
populate({ a: [] });
expect(subject.child('a').exists()).to.be.false;

populate({ a: [null] });
expect(subject.child('a').exists()).to.be.false;

populate({ a: [{}] });
expect(subject.child('a').exists()).to.be.false;
});
});

describe('#forEach(action: (a: DataSnapshot) => boolean): boolean', () => {
Expand Down Expand Up @@ -614,6 +672,17 @@ describe('Database Functions', () => {

expect(subject.forEach(counter)).to.equal(false);
expect(count).to.eq(0);

populate({
a: 'foo',
nullChild: null,
emptyObjectChild: {},
emptyArrayChild: [],
});
count = 0;

expect(subject.forEach(counter)).to.equal(false);
expect(count).to.eq(1);
});

it('should cancel further enumeration if callback returns true', () => {
Expand Down Expand Up @@ -653,13 +722,51 @@ describe('Database Functions', () => {

describe('#numChildren()', () => {
it('should be key count for objects', () => {
populate({ a: 'b', c: 'd' });
populate({
a: 'b',
c: 'd',
nullChild: null,
emptyObjectChild: {},
emptyArrayChild: [],
});
expect(subject.numChildren()).to.eq(2);
});

it('should be 0 for non-objects', () => {
populate(23);
expect(subject.numChildren()).to.eq(0);

populate({
nullChild: null,
emptyObjectChild: {},
emptyArrayChild: [],
});
expect(subject.numChildren()).to.eq(0);
});
});

describe('#hasChildren()', () => {
it('should true for objects', () => {
populate({
a: 'b',
c: 'd',
nullChild: null,
emptyObjectChild: {},
emptyArrayChild: [],
});
expect(subject.hasChildren()).to.be.true;
});

it('should be false for non-objects', () => {
populate(23);
expect(subject.hasChildren()).to.be.false;

populate({
nullChild: null,
emptyObjectChild: {},
emptyArrayChild: [],
});
expect(subject.hasChildren()).to.be.false;
Comment on lines +748 to +769
Copy link
Contributor Author

@rhodgkins rhodgkins Jul 31, 2021 •
edited

Choose a reason for hiding this comment

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

.hasChildren() had no tests so added some by mirroring the .numChildren() tests above.

});
});

Expand All @@ -671,9 +778,17 @@ describe('Database Functions', () => {
});

it('should return false if a child is missing', () => {
populate({ a: 'b' });
populate({
a: 'b',
nullChild: null,
emptyObjectChild: {},
emptyArrayChild: [],
});
expect(subject.hasChild('c')).to.be.false;
expect(subject.hasChild('a/b')).to.be.false;
expect(subject.hasChild('nullChild')).to.be.false;
expect(subject.hasChild('emptyObjectChild')).to.be.false;
expect(subject.hasChild('emptyArrayChild')).to.be.false;
});
});

Expand Down Expand Up @@ -703,11 +818,21 @@ describe('Database Functions', () => {

describe('#toJSON(): Object', () => {
it('should return the current value', () => {
populate({ a: 'b' });
populate({
a: 'b',
nullChild: null,
emptyObjectChild: {},
emptyArrayChild: [],
});
expect(subject.toJSON()).to.deep.equal(subject.val());
});
it('should be stringifyable', () => {
populate({ a: 'b' });
populate({
a: 'b',
nullChild: null,
emptyObjectChild: {},
emptyArrayChild: [],
});
expect(JSON.stringify(subject)).to.deep.equal('{"a":"b"}');
});
});
Expand Down
29 changes: 24 additions & 5 deletions src/v1/providers/database.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,16 @@ export class DataSnapshot {
* @return `true` if this `DataSnapshot` contains any data; otherwise, `false`.
*/
exists(): boolean {
return !_.isNull(this.val());
const val = this.val();
if (_.isNull(val)) {
// Null value
return false;
}
if (_.isObjectLike(val) && _.isEmpty(val)) {
// Empty object/array
return false;
}
return true;
}

/**
Expand Down Expand Up @@ -512,7 +521,7 @@ export class DataSnapshot {
*/
forEach(action: (a: DataSnapshot) => boolean | void): boolean {
const val = this.val();
if (_.isPlainObject(val)) {
if (_.isObjectLike(val)) {
return _.some(
val,
(value, key: string) => action(this.child(key)) === true
Expand Down Expand Up @@ -546,7 +555,7 @@ export class DataSnapshot {
*/
hasChildren(): boolean {
const val = this.val();
return _.isPlainObject(val) && _.keys(val).length > 0;
return _.isObjectLike(val) && !_.isEmpty(val);
}

/**
Expand All @@ -556,7 +565,7 @@ export class DataSnapshot {
*/
numChildren(): number {
const val = this.val();
return _.isPlainObject(val) ? Object.keys(val).length : 0;
return _.isObjectLike(val) ? _.keys(val).length : 0;
}

/**
Expand Down Expand Up @@ -588,7 +597,12 @@ export class DataSnapshot {
continue;
}
const childNode = node[key];
obj[key] = this._checkAndConvertToArray(childNode);
const v = this._checkAndConvertToArray(childNode);
if (v === null) {
// Empty child node
continue;
}
obj[key] = v;
numKeys++;
const integerRegExp = /^(0|[1-9]\d*)$/;
if (allIntegerKeys && integerRegExp.test(key)) {
Expand All @@ -598,6 +612,11 @@ export class DataSnapshot {
}
}

if (numKeys === 0) {
// Empty node
return null;
}

Comment on lines +615 to +619
Copy link
Contributor Author

@rhodgkins rhodgkins Jul 31, 2021 •
edited

Choose a reason for hiding this comment

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

This simulates the previously mentioned this.isEmpty() check the Firebase SDK does.

if (allIntegerKeys && maxKey < 2 * numKeys) {
// convert to array.
const array: any = [];
Expand Down