Skip to content

Add isEqual() and port DocumentReference array test. #222

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

Merged
merged 1 commit into from
Oct 16, 2017
Merged

Add isEqual() and port DocumentReference array test. #222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions packages/firebase/app/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -969,6 +969,14 @@ declare namespace firebase.firestore {
*/
collection(collectionPath: string): CollectionReference;

/**
* Returns true if this `DocumentReference` is equal to the provided one.
*
* @param other The `DocumentReference` to compare against.
* @return true if this `DocumentReference` is equal to the provided one.
*/
isEqual(other: DocumentReference): boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is gas on a fire, but why isEqual? Within our codebase I see usages like these:
equals 187
isEqual 53

Looking more closely, the 53 usages of isEqual are just database's Query.isEqual and its tests (some some checked in logs and Firestore's Query.isEqual).

It seems far more conventional to call this method equals.

(BTW: I'm finding usages like this)

find . \( -name node_modules \
    -o -name dist \
    -o -name \*.js \
    -o -name \*.map \) -prune -o -type f -print0 \
  | xargs -0 grep isEqual

Copy link
Contributor

@wilhuff wilhuff Oct 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, discussed offline, and it's clear that there's no consensus in the world about isEqual vs equals. The much larger incidence of equals within our code are internal APIs.

Let's file a clean up bug to standardize on isEqual so that customDeepEqual need not check both.


/**
* Writes to the document referred to by this `DocumentReference`. If the
* document does not yet exist, it will be created. If you pass
Expand Down Expand Up @@ -1308,6 +1316,14 @@ declare namespace firebase.firestore {
*/
endAt(...fieldValues: any[]): Query;

/**
* Returns true if this `Query` is equal to the provided one.
*
* @param other The `Query` to compare against.
* @return true if this `Query` is equal to the provided one.
*/
isEqual(other: Query): boolean;

/**
* Executes the query and returns the results as a QuerySnapshot.
*
Expand Down
16 changes: 16 additions & 0 deletions packages/firebase/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -969,6 +969,14 @@ declare namespace firebase.firestore {
*/
collection(collectionPath: string): CollectionReference;

/**
* Returns true if this `DocumentReference` is equal to the provided one.
*
* @param other The `DocumentReference` to compare against.
* @return true if this `DocumentReference` is equal to the provided one.
*/
isEqual(other: DocumentReference): boolean;

/**
* Writes to the document referred to by this `DocumentReference`. If the
* document does not yet exist, it will be created. If you pass
Expand Down Expand Up @@ -1308,6 +1316,14 @@ declare namespace firebase.firestore {
*/
endAt(...fieldValues: any[]): Query;

/**
* Returns true if this `Query` is equal to the provided one.
*
* @param other The `Query` to compare against.
* @return true if this `Query` is equal to the provided one.
*/
isEqual(other: Query): boolean;

/**
* Executes the query and returns the results as a QuerySnapshot.
*
Expand Down
16 changes: 16 additions & 0 deletions packages/firestore/src/api/database.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,6 +742,13 @@ export class DocumentReference implements firestore.DocumentReference {
return new CollectionReference(this._key.path.child(path), this.firestore);
}

isEqual(other: firestore.DocumentReference): boolean {
if (!(other instanceof DocumentReference)) {
throw invalidClassError('isEqual', 'DocumentReference', 1, other);
}
return this.firestore === other.firestore && this._key.equals(other._key);
}

set(
value: firestore.DocumentData,
options?: firestore.SetOptions
Expand Down Expand Up @@ -1240,6 +1247,15 @@ export class Query implements firestore.Query {
return new Query(this._query.withEndAt(bound), this.firestore);
}

isEqual(other: firestore.Query): boolean {
if (!(other instanceof Query)) {
throw invalidClassError('isEqual', 'Query', 1, other);
}
return (
this.firestore === other.firestore && this._query.equals(other._query)
);
}

/** Helper function to create a bound from a document or fields */
private boundFromDocOrFields(
methodName: string,
Expand Down
16 changes: 16 additions & 0 deletions packages/firestore/src/typings/firestore.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,14 @@ declare namespace firestore {
*/
collection(collectionPath: string): CollectionReference;

/**
* Returns true if this `DocumentReference` is equal to the provided one.
*
* @param other The `DocumentReference` to compare against.
* @return true if this `DocumentReference` is equal to the provided one.
*/
isEqual(other: DocumentReference): boolean;

/**
* Writes to the document referred to by this `DocumentReference`. If the
* document does not yet exist, it will be created. If you pass
Expand Down Expand Up @@ -742,6 +750,14 @@ declare namespace firestore {
*/
endAt(...fieldValues: any[]): Query;

/**
* Returns true if this `Query` is equal to the provided one.
*
* @param other The `Query` to compare against.
* @return true if this `Query` is equal to the provided one.
*/
isEqual(other: Query): boolean;

/**
* Executes the query and returns the results as a QuerySnapshot.
*
Expand Down
50 changes: 48 additions & 2 deletions packages/firestore/test/integration/api/database.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,20 +468,66 @@ apiDescribe('Database', persistence => {
});
});

asyncIt('exposes "database" on document references.', () => {
asyncIt('exposes "firestore" on document references.', () => {
return withTestDb(persistence, db => {
expect(db.doc('foo/bar').firestore).to.equal(db);
return Promise.resolve();
});
});

asyncIt('exposes "database" on query references.', () => {
asyncIt('exposes "firestore" on query references.', () => {
return withTestDb(persistence, db => {
expect(db.collection('foo').limit(5).firestore).to.equal(db);
return Promise.resolve();
});
});

asyncIt('can compare DocumentReference instances with isEqual().', () => {
return withTestDb(persistence, firestore => {
return withTestDb(persistence, otherFirestore => {
const docRef = firestore.doc('foo/bar');
expect(docRef.isEqual(firestore.doc('foo/bar'))).to.be.true;
expect(docRef.collection('baz').parent.isEqual(docRef)).to.be.true;

expect(firestore.doc('foo/BAR').isEqual(docRef)).to.be.false;

expect(otherFirestore.doc('foo/bar').isEqual(docRef)).to.be.false;

return Promise.resolve();
});
});
});

asyncIt('can compare Query instances with isEqual().', () => {
return withTestDb(persistence, firestore => {
return withTestDb(persistence, otherFirestore => {
const query = firestore
.collection('foo')
.orderBy('bar')
.where('baz', '==', 42);
const query2 = firestore
.collection('foo')
.orderBy('bar')
.where('baz', '==', 42);
expect(query.isEqual(query2)).to.be.true;

const query3 = firestore
.collection('foo')
.orderBy('BAR')
.where('baz', '==', 42);
expect(query.isEqual(query3)).to.be.false;

const query4 = otherFirestore
.collection('foo')
.orderBy('bar')
.where('baz', '==', 42);
expect(query4.isEqual(query)).to.be.false;

return Promise.resolve();
});
});
});

asyncIt('can traverse collections and documents.', () => {
return withTestDb(persistence, db => {
const expected = 'a/b/c/d';
Expand Down
20 changes: 7 additions & 13 deletions packages/firestore/test/integration/api/type.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,20 +93,14 @@ apiDescribe('Firestore', persistence => {
});

asyncIt('can read and write document references', () => {
// TODO(b/62143881): Implement DocumentReference.equals() so we can use
// expectRoundtrip()
return withTestDoc(persistence, doc => {
return doc
.set({ a: 42, ref: doc })
.then(() => {
return doc.get();
})
.then(docSnap => {
expect(docSnap.exists).to.equal(true);
expect(docSnap.get('a')).to.equal(42);
const readDocRef = docSnap.get('ref') as firestore.DocumentReference;
expect(readDocRef.path).to.equal(doc.path);
});
return expectRoundtrip(doc.firestore, { a: 42, ref: doc });
});
});

asyncIt('can read and write document references in an array', () => {
return withTestDoc(persistence, doc => {
return expectRoundtrip(doc.firestore, { a: 42, refs: [doc] });
});
});
});
4 changes: 4 additions & 0 deletions packages/firestore/test/util/equality_matcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,12 @@ function customDeepEqual(left, right) {
/**
* START: Custom compare logic
*/
// Internally we use .equals(), but our public API uses .isEqual() so
// we handle both.
if (left && typeof left.equals === 'function') return left.equals(right);
if (left && typeof left.isEqual === 'function') return left.isEqual(right);
if (right && typeof right.equals === 'function') return right.equals(left);
if (right && typeof right.isEqual === 'function') return right.isEqual(left);
/**
* END: Custom compare logic
*/
Expand Down