Skip to content

Make Converter passing explicit #3171

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

Merged
merged 4 commits into from
Jun 8, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 16 additions & 7 deletions packages/firestore/lite/src/api/reference.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export class DocumentReference<T = firestore.DocumentData>
constructor(
readonly firestore: Firestore,
key: DocumentKey,
readonly _converter?: firestore.FirestoreDataConverter<T>
readonly _converter: firestore.FirestoreDataConverter<T> | null
) {
super(firestore._databaseId, key, _converter);
}
Expand All @@ -79,7 +79,7 @@ export class Query<T = firestore.DocumentData> implements firestore.Query<T> {
constructor(
readonly firestore: Firestore,
readonly _query: InternalQuery,
readonly _converter?: firestore.FirestoreDataConverter<T>
readonly _converter: firestore.FirestoreDataConverter<T> | null
) {}

where(
Expand Down Expand Up @@ -153,7 +153,7 @@ export class CollectionReference<T = firestore.DocumentData> extends Query<T>
constructor(
readonly firestore: Firestore,
readonly _path: ResourcePath,
readonly _converter?: firestore.FirestoreDataConverter<T>
readonly _converter: firestore.FirestoreDataConverter<T> | null
) {
super(firestore, InternalQuery.atPath(_path), _converter);
}
Expand Down Expand Up @@ -189,12 +189,16 @@ export function collection(
const path = ResourcePath.fromString(relativePath);
if (parent instanceof Firestore) {
validateCollectionPath(path);
return new CollectionReference(parent, path);
return new CollectionReference(parent, path, /* converter= */ null);
} else {
const doc = cast(parent, DocumentReference);
const absolutePath = doc._key.path.child(path);
validateCollectionPath(absolutePath);
return new CollectionReference(doc.firestore, absolutePath);
return new CollectionReference(
doc.firestore,
absolutePath,
/* converter= */ null
);
}
}

Expand All @@ -219,7 +223,11 @@ export function doc<T>(
const path = ResourcePath.fromString(relativePath!);
if (parent instanceof Firestore) {
validateDocumentPath(path);
return new DocumentReference(parent, new DocumentKey(path));
return new DocumentReference(
parent,
new DocumentKey(path),
/* converter= */ null
);
} else {
const coll = cast(parent, CollectionReference);
const absolutePath = coll._path.child(path);
Expand Down Expand Up @@ -248,7 +256,8 @@ export function parent<T>(
} else {
return new DocumentReference(
child.firestore,
new DocumentKey(parentPath)
new DocumentKey(parentPath),
/* converter= */ null
);
}
} else {
Expand Down
8 changes: 5 additions & 3 deletions packages/firestore/lite/src/api/snapshot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export class DocumentSnapshot<T = firestore.DocumentData>
private _firestore: Firestore,
private _key: DocumentKey,
private _document: Document | null,
private _converter?: firestore.FirestoreDataConverter<T>
private _converter: firestore.FirestoreDataConverter<T> | null
) {}

get id(): string {
Expand Down Expand Up @@ -66,15 +66,17 @@ export class DocumentSnapshot<T = firestore.DocumentData>
const snapshot = new QueryDocumentSnapshot(
this._firestore,
this._key,
this._document
this._document,
/* converter= */ null
);
return this._converter.fromFirestore(snapshot);
} else {
const userDataWriter = new UserDataWriter(
this._firestore._databaseId,
/* timestampsInSnapshots= */ false,
/* serverTimestampBehavior=*/ 'none',
key => new DocumentReference(this._firestore, key)
key =>
new DocumentReference(this._firestore, key, /* converter= */ null)
);
return userDataWriter.convertValue(this._document.toProto()) as T;
}
Expand Down
46 changes: 31 additions & 15 deletions packages/firestore/src/api/database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -580,14 +580,22 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
validateExactNumberOfArgs('Firestore.collection', arguments, 1);
validateArgType('Firestore.collection', 'non-empty string', 1, pathString);
this.ensureClientConfigured();
return new CollectionReference(ResourcePath.fromString(pathString), this);
return new CollectionReference(
ResourcePath.fromString(pathString),
this,
/* converter= */ null
);
}

doc(pathString: string): firestore.DocumentReference {
validateExactNumberOfArgs('Firestore.doc', arguments, 1);
validateArgType('Firestore.doc', 'non-empty string', 1, pathString);
this.ensureClientConfigured();
return DocumentReference.forPath(ResourcePath.fromString(pathString), this);
return DocumentReference.forPath(
ResourcePath.fromString(pathString),
this,
/* converter= */ null
);
}

collectionGroup(collectionId: string): firestore.Query {
Expand All @@ -608,7 +616,8 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
this.ensureClientConfigured();
return new Query(
new InternalQuery(ResourcePath.EMPTY_PATH, collectionId),
this
this,
/* converter= */ null
);
}

Expand Down Expand Up @@ -946,7 +955,7 @@ export class DocumentReference<T = firestore.DocumentData>
constructor(
public _key: DocumentKey,
readonly firestore: Firestore,
readonly _converter?: firestore.FirestoreDataConverter<T>
readonly _converter: firestore.FirestoreDataConverter<T> | null
) {
super(firestore._databaseId, _key, _converter);
this._firestoreClient = this.firestore.ensureClientConfigured();
Expand All @@ -955,7 +964,7 @@ export class DocumentReference<T = firestore.DocumentData>
static forPath<U>(
path: ResourcePath,
firestore: Firestore,
converter?: firestore.FirestoreDataConverter<U>
converter: firestore.FirestoreDataConverter<U> | null
): DocumentReference<U> {
if (path.length % 2 !== 0) {
throw new FirestoreError(
Expand Down Expand Up @@ -1001,7 +1010,11 @@ export class DocumentReference<T = firestore.DocumentData>
);
}
const path = ResourcePath.fromString(pathString);
return new CollectionReference(this._key.path.child(path), this.firestore);
return new CollectionReference(
this._key.path.child(path),
this.firestore,
/* converter= */ null
);
}

isEqual(other: firestore.DocumentReference<T>): boolean {
Expand Down Expand Up @@ -1328,7 +1341,7 @@ export class DocumentSnapshot<T = firestore.DocumentData>
public _document: Document | null,
private _fromCache: boolean,
private _hasPendingWrites: boolean,
private readonly _converter?: firestore.FirestoreDataConverter<T>
private readonly _converter: firestore.FirestoreDataConverter<T> | null
) {}

data(options?: firestore.SnapshotOptions): T | undefined {
Expand All @@ -1345,15 +1358,17 @@ export class DocumentSnapshot<T = firestore.DocumentData>
this._key,
this._document,
this._fromCache,
this._hasPendingWrites
this._hasPendingWrites,
/* converter= */ null
);
return this._converter.fromFirestore(snapshot, options);
} else {
const userDataWriter = new UserDataWriter(
this._firestore._databaseId,
this._firestore._areTimestampsInSnapshotsEnabled(),
options.serverTimestamps || 'none',
key => new DocumentReference(key, this._firestore)
key =>
new DocumentReference(key, this._firestore, /* converter= */ null)
);
return userDataWriter.convertValue(this._document.toProto()) as T;
}
Expand Down Expand Up @@ -1436,7 +1451,7 @@ export class Query<T = firestore.DocumentData> implements firestore.Query<T> {
constructor(
public _query: InternalQuery,
readonly firestore: Firestore,
protected readonly _converter?: firestore.FirestoreDataConverter<T>
protected readonly _converter: firestore.FirestoreDataConverter<T> | null
) {}

where(
Expand Down Expand Up @@ -2168,7 +2183,7 @@ export class QuerySnapshot<T = firestore.DocumentData>
private readonly _firestore: Firestore,
private readonly _originalQuery: InternalQuery,
private readonly _snapshot: ViewSnapshot,
private readonly _converter?: firestore.FirestoreDataConverter<T>
private readonly _converter: firestore.FirestoreDataConverter<T> | null
) {
this.metadata = new SnapshotMetadata(
_snapshot.hasPendingWrites,
Expand Down Expand Up @@ -2279,7 +2294,7 @@ export class CollectionReference<T = firestore.DocumentData> extends Query<T>
constructor(
readonly _path: ResourcePath,
firestore: Firestore,
_converter?: firestore.FirestoreDataConverter<T>
_converter: firestore.FirestoreDataConverter<T> | null
) {
super(InternalQuery.atPath(_path), firestore, _converter);
if (_path.length % 2 !== 1) {
Expand All @@ -2303,7 +2318,8 @@ export class CollectionReference<T = firestore.DocumentData> extends Query<T>
} else {
return new DocumentReference<firestore.DocumentData>(
new DocumentKey(parentPath),
this.firestore
this.firestore,
/* converter= */ null
);
}
}
Expand Down Expand Up @@ -2444,7 +2460,7 @@ export function changesFromSnapshot<T>(
firestore: Firestore,
includeMetadataChanges: boolean,
snapshot: ViewSnapshot,
converter?: firestore.FirestoreDataConverter<T>
converter: firestore.FirestoreDataConverter<T> | null
): Array<firestore.DocumentChange<T>> {
if (snapshot.oldDocs.isEmpty()) {
// Special case the first snapshot because index calculation is easy and
Expand Down Expand Up @@ -2533,7 +2549,7 @@ function resultChangeType(type: ChangeType): firestore.DocumentChangeType {
* call.
*/
export function applyFirestoreDataConverter<T>(
converter: UntypedFirestoreDataConverter<T> | undefined,
converter: UntypedFirestoreDataConverter<T> | null,
value: T,
functionName: string
): [firestore.DocumentData, string] {
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/src/api/user_data_reader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export class DocumentKeyReference<T> {
constructor(
public readonly _databaseId: DatabaseId,
public readonly _key: DocumentKey,
public readonly _converter?: UntypedFirestoreDataConverter<T>
public readonly _converter: UntypedFirestoreDataConverter<T> | null
) {}
}

Expand Down
7 changes: 6 additions & 1 deletion packages/firestore/test/unit/api/document_change.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,12 @@ describe('DocumentChange:', () => {
const expected = documentSetAsArray(updatedSnapshot.docs);
const actual = documentSetAsArray(initialSnapshot.docs);

const changes = changesFromSnapshot({} as Firestore, true, updatedSnapshot);
const changes = changesFromSnapshot(
{} as Firestore,
true,
updatedSnapshot,
/* converter= */ null
);

for (const change of changes) {
if (change.type !== 'added') {
Expand Down
31 changes: 23 additions & 8 deletions packages/firestore/test/util/api_helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ import { Provider, ComponentContainer } from '@firebase/component';
*/
export const FIRESTORE = new Firestore(
{
projectId: 'projectid',
database: 'database'
projectId: 'test-project',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed so I can get rid of fakeFirestore.

database: '(default)'
},
new Provider('auth-internal', new ComponentContainer('default')),
new IndexedDbComponentProvider()
Expand All @@ -57,11 +57,15 @@ export function firestore(): Firestore {
}

export function collectionReference(path: string): CollectionReference {
return new CollectionReference(pathFrom(path), firestore());
return new CollectionReference(
pathFrom(path),
firestore(),
/* converter= */ null
);
}

export function documentReference(path: string): DocumentReference {
return new DocumentReference(key(path), firestore());
return new DocumentReference(key(path), firestore(), /* converter= */ null);
}

export function documentSnapshot(
Expand All @@ -75,21 +79,27 @@ export function documentSnapshot(
key(path),
doc(path, 1, data),
fromCache,
/* hasPendingWrites= */ false
/* hasPendingWrites= */ false,
/* converter= */ null
);
} else {
return new DocumentSnapshot(
firestore(),
key(path),
null,
fromCache,
/* hasPendingWrites= */ false
/* hasPendingWrites= */ false,
/* converter= */ null
);
}
}

export function query(path: string): Query {
return new Query(InternalQuery.atPath(pathFrom(path)), firestore());
return new Query(
InternalQuery.atPath(pathFrom(path)),
firestore(),
/* converter= */ null
);
}

/**
Expand Down Expand Up @@ -135,5 +145,10 @@ export function querySnapshot(
syncStateChanged,
false
);
return new QuerySnapshot(firestore(), query, viewSnapshot);
return new QuerySnapshot(
firestore(),
query,
viewSnapshot,
/* converter= */ null
);
}
16 changes: 5 additions & 11 deletions packages/firestore/test/util/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,32 +84,25 @@ import { primitiveComparator } from '../../src/util/misc';
import { Dict, forEach } from '../../src/util/obj';
import { SortedMap } from '../../src/util/sorted_map';
import { SortedSet } from '../../src/util/sorted_set';
import { query } from './api_helpers';
import { FIRESTORE, query } from './api_helpers';
import { ByteString } from '../../src/util/byte_string';
import { PlatformSupport } from '../../src/platform/platform';
import { JsonProtoSerializer } from '../../src/remote/serializer';
import { Timestamp } from '../../src/api/timestamp';
import { DocumentReference, Firestore } from '../../src/api/database';
import { DocumentReference } from '../../src/api/database';
import { DeleteFieldValueImpl } from '../../src/api/field_value';
import { Code, FirestoreError } from '../../src/util/error';

/* eslint-disable no-restricted-globals */

// A Firestore that can be used in DocumentReferences and UserDataWriter.
const fakeFirestore: Firestore = {
ensureClientConfigured: () => {},
_databaseId: new DatabaseId('test-project')
// eslint-disable-next-line @typescript-eslint/no-explicit-any
} as any;

export type TestSnapshotVersion = number;

export function testUserDataWriter(): UserDataWriter {
return new UserDataWriter(
new DatabaseId('test-project'),
/* timestampsInSnapshots= */ false,
'none',
key => new DocumentReference(key, fakeFirestore)
key => new DocumentReference(key, FIRESTORE, /* converter= */ null)
);
}

Expand All @@ -133,7 +126,8 @@ export function version(v: TestSnapshotVersion): SnapshotVersion {
export function ref(key: string, offset?: number): DocumentReference {
return new DocumentReference(
new DocumentKey(path(key, offset)),
fakeFirestore
FIRESTORE,
/* converter= */ null
);
}

Expand Down