Skip to content

Add support for collection(coll) and doc(doc) #3403

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 2 commits into from
Jul 16, 2020
Merged

Add support for collection(coll) and doc(doc) #3403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9c4572
Add support for collection(coll) and doc(doc)
schmidt-sebastian Jul 14, 2020
10f89b2
Add test
schmidt-sebastian Jul 16, 2020
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
8 changes: 8 additions & 0 deletions packages/firestore/exp/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,10 @@ export function collection(
firestore: FirebaseFirestore,
collectionPath: string
): CollectionReference<DocumentData>;
export function collection(
reference: CollectionReference<unknown>,
collectionPath: string
): CollectionReference<DocumentData>;
export function collection(
reference: DocumentReference,
collectionPath: string
Expand All @@ -121,6 +125,10 @@ export function doc<T>(
reference: CollectionReference<T>,
documentPath?: string
): DocumentReference<T>;
export function doc(
reference: DocumentReference<unknown>,
documentPath: string
): DocumentReference<DocumentData>;
export function parent(
reference: CollectionReference<unknown>
): DocumentReference<DocumentData> | null;
Expand Down
8 changes: 8 additions & 0 deletions packages/firestore/lite/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ export function collection(
firestore: FirebaseFirestore,
collectionPath: string
): CollectionReference<DocumentData>;
export function collection(
reference: CollectionReference<unknown>,
collectionPath: string
): CollectionReference<DocumentData>;
export function collection(
reference: DocumentReference,
collectionPath: string
Expand All @@ -82,6 +86,10 @@ export function doc<T>(
reference: CollectionReference<T>,
documentPath?: string
): DocumentReference<T>;
export function doc(
reference: DocumentReference<unknown>,
documentPath: string
): DocumentReference<DocumentData>;
export function parent(
reference: CollectionReference<unknown>
): DocumentReference<DocumentData> | null;
Expand Down
67 changes: 52 additions & 15 deletions packages/firestore/lite/src/api/reference.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,25 +283,43 @@ export function collection(
firestore: firestore.FirebaseFirestore,
collectionPath: string
): CollectionReference<firestore.DocumentData>;
export function collection(
reference: firestore.CollectionReference<unknown>,
collectionPath: string
): CollectionReference<firestore.DocumentData>;
export function collection(
reference: firestore.DocumentReference,
collectionPath: string
): CollectionReference<firestore.DocumentData>;
export function collection(
parent: firestore.FirebaseFirestore | firestore.DocumentReference<unknown>,
parent:
| firestore.FirebaseFirestore
| firestore.DocumentReference<unknown>
| firestore.CollectionReference<unknown>,
relativePath: string
): CollectionReference<firestore.DocumentData> {
validateArgType('collection', 'non-empty string', 2, relativePath);
const path = ResourcePath.fromString(relativePath);
if (parent instanceof Firestore) {
validateCollectionPath(path);
return new CollectionReference(parent, path, /* converter= */ null);
const absolutePath = ResourcePath.fromString(relativePath);
validateCollectionPath(absolutePath);
return new CollectionReference(parent, absolutePath, /* converter= */ null);
} else {
const doc = cast(parent, DocumentReference);
const absolutePath = doc._key.path.child(path);
if (
Copy link

Choose a reason for hiding this comment

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

When would we hit this error block in TS since the arguments are typed? Or is this for JS users? How could you test that this works properly in TS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be hit if someone passes a DocumentReference-looking type into our SDK. Our external types only validate that type passed in looks like a DocumentReference (has all the same properties) but they cannot enforce that the provided type is an actual instance of DocumentReference.

!(parent instanceof DocumentReference) &&
!(parent instanceof CollectionReference)
) {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
'Expected first argument to collection() to be a CollectionReference, ' +
'a DocumentReference or FirebaseFirestore'
);
}
const absolutePath = ResourcePath.fromString(
`${parent.path}/${relativePath}`
);
validateCollectionPath(absolutePath);
return new CollectionReference(
doc.firestore,
parent.firestore,
absolutePath,
/* converter= */ null
);
Expand Down Expand Up @@ -340,8 +358,15 @@ export function doc<T>(
reference: firestore.CollectionReference<T>,
documentPath?: string
): DocumentReference<T>;
export function doc(
reference: firestore.DocumentReference<unknown>,
documentPath: string
): DocumentReference<firestore.DocumentData>;
export function doc<T>(
parent: firestore.FirebaseFirestore | firestore.CollectionReference<T>,
parent:
| firestore.FirebaseFirestore
| firestore.CollectionReference<T>
| firestore.DocumentReference<unknown>,
relativePath?: string
): DocumentReference {
// We allow omission of 'pathString' but explicitly prohibit passing in both
Expand All @@ -350,22 +375,34 @@ export function doc<T>(
relativePath = AutoId.newId();
}
validateArgType('doc', 'non-empty string', 2, relativePath);
const path = ResourcePath.fromString(relativePath!);

if (parent instanceof Firestore) {
validateDocumentPath(path);
const absolutePath = ResourcePath.fromString(relativePath!);
validateDocumentPath(absolutePath);
return new DocumentReference(
parent,
new DocumentKey(path),
new DocumentKey(absolutePath),
/* converter= */ null
);
} else {
const coll = cast(parent, CollectionReference);
const absolutePath = coll._path.child(path);
if (
!(parent instanceof DocumentReference) &&
!(parent instanceof CollectionReference)
) {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
'Expected first argument to collection() to be a CollectionReference, ' +
'a DocumentReference or FirebaseFirestore'
);
}
const absolutePath = ResourcePath.fromString(
`${parent.path}/${relativePath}`
);
validateDocumentPath(absolutePath);
return new DocumentReference(
coll.firestore,
parent.firestore,
new DocumentKey(absolutePath),
coll._converter
parent instanceof CollectionReference ? parent._converter : null
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 a test to make sure the converter is properly passed/omitted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

);
}
}
Expand Down
55 changes: 50 additions & 5 deletions packages/firestore/lite/test/integration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ describe('Firestore', () => {
});

describe('doc', () => {
it('can provide name', () => {
it('can be used relative to Firestore root', () => {
return withTestDb(db => {
const result = doc(db, 'coll/doc');
expect(result).to.be.an.instanceOf(DocumentReference);
Expand All @@ -152,6 +152,24 @@ describe('doc', () => {
});
});

it('can be used relative to collection', () => {
return withTestDb(db => {
const result = doc(collection(db, 'coll'), 'doc');
expect(result).to.be.an.instanceOf(DocumentReference);
expect(result.id).to.equal('doc');
expect(result.path).to.equal('coll/doc');
});
});

it('can be relative to doc', () => {
return withTestDb(db => {
const result = doc(doc(db, 'coll/doc'), 'subcoll/subdoc');
expect(result).to.be.an.instanceOf(DocumentReference);
expect(result.id).to.equal('subdoc');
expect(result.path).to.equal('coll/doc/subcoll/subdoc');
});
});

it('validates path', () => {
return withTestDb(db => {
expect(() => doc(db, 'coll')).to.throw(
Expand Down Expand Up @@ -179,7 +197,7 @@ describe('doc', () => {
});

describe('collection', () => {
it('can provide name', () => {
it('can be used relative to Firestore root', () => {
return withTestDb(db => {
const result = collection(db, 'coll/doc/subcoll');
expect(result).to.be.an.instanceOf(CollectionReference);
Expand All @@ -188,6 +206,24 @@ describe('collection', () => {
});
});

it('can be used relative to collection', () => {
return withTestDb(db => {
const result = collection(collection(db, 'coll'), 'doc/subcoll');
expect(result).to.be.an.instanceOf(CollectionReference);
expect(result.id).to.equal('subcoll');
expect(result.path).to.equal('coll/doc/subcoll');
});
});

it('can be used relative to doc', () => {
return withTestDb(db => {
const result = collection(doc(db, 'coll/doc'), 'subcoll');
expect(result).to.be.an.instanceOf(CollectionReference);
expect(result.id).to.equal('subcoll');
expect(result.path).to.equal('coll/doc/subcoll');
});
});

it('validates path', () => {
return withTestDb(db => {
expect(() => collection(db, 'coll/doc')).to.throw(
Expand All @@ -196,7 +232,7 @@ describe('collection', () => {
// TODO(firestorelite): Explore returning a more helpful message
// (e.g. "Empty document paths are not supported.")
expect(() => collection(doc(db, 'coll/doc'), '')).to.throw(
'Function doc() requires its second argument to be of type non-empty string, but it was: ""'
'Function collection() requires its second argument to be of type non-empty string, but it was: ""'
);
expect(() => collection(doc(db, 'coll/doc'), 'coll/doc')).to.throw(
'Invalid collection path (coll/doc/coll/doc). Path points to a document.'
Expand Down Expand Up @@ -945,8 +981,7 @@ describe('equality', () => {
const query1a = collRef.orderBy('foo');
const query1b = collRef.orderBy('foo', 'asc');
const query2 = collRef.orderBy('foo', 'desc');
// TODO(firestorelite): Should we allow `collectionRef(collRef, 'a/b')?
const query3 = collection(doc(collRef, 'a'), 'b').orderBy('foo');
const query3 = collection(collRef, 'a/b').orderBy('foo');

expect(queryEqual(query1a, query1b)).to.be.true;
expect(queryEqual(query1a, query2)).to.be.false;
Expand Down Expand Up @@ -1029,6 +1064,16 @@ describe('withConverter() support', () => {
});
});

it('keeps the converter when calling parent() with a DocumentReference', () => {
return withTestDb(async db => {
const coll = doc(db, 'root/doc').withConverter(postConverter);
const typedColl = parent(coll)!;
expect(
refEqual(typedColl, collection(db, 'root').withConverter(postConverter))
).to.be.true;
});
});

it('drops the converter when calling parent() with a CollectionReference', () => {
return withTestDb(async db => {
const coll = collection(db, 'root/doc/parent').withConverter(
Expand Down