Skip to content

Finish FieldValue migration #2766

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 9 commits into from
Mar 23, 2020
Merged

Finish FieldValue migration #2766

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3d7c892
Replace FieldValue with ProtoFieldValue
schmidt-sebastian Mar 19, 2020
b6a60a3
Finish FieldValue migration
schmidt-sebastian Mar 19, 2020
c7613ac
Misc fixes
schmidt-sebastian Mar 21, 2020
746eca1
Equals and compare rename
schmidt-sebastian Mar 21, 2020
d13e5be
Remove isType
schmidt-sebastian Mar 21, 2020
985d514
Add a ServerTimestamp type order
schmidt-sebastian Mar 21, 2020
a0d0363
Re-add warning for foreign keys
schmidt-sebastian Mar 21, 2020
2cd4864
Fix CI
schmidt-sebastian Mar 21, 2020
309b84a
Review
schmidt-sebastian Mar 23, 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
52 changes: 25 additions & 27 deletions packages/firestore/src/api/database.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@

import * as firestore from '@firebase/firestore-types';

import * as api from '../protos/firestore_proto_api';
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought you wanted to get rid of these * imports. Or will we tackle that in a separate PR?

In this case I think you could make this without causing too much pain.

import { Value } from '../protos/firestore_proto_api';

This seems like it applies throughout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.d.ts files actually contain no JavaScript code and don't affect bundle size. We can remove these imports for consistency, but that leads to the terribly non-descriptive names of Value and Timestamp (unless we rename on import). I vote we keep this as is for now and discuss in a follow up.


import { FirebaseApp } from '@firebase/app-types';
import { FirebaseService, _FirebaseApp } from '@firebase/app-types/private';
import { _FirebaseApp, FirebaseService } from '@firebase/app-types/private';
import { DatabaseId, DatabaseInfo } from '../core/database_info';
import { ListenOptions } from '../core/event_manager';
import { FirestoreClient, PersistenceSettings } from '../core/firestore_client';
Expand All @@ -38,14 +40,11 @@ import { MemoryPersistenceProvider } from '../local/memory_persistence';
import { PersistenceProvider } from '../local/persistence';
import { Document, MaybeDocument, NoDocument } from '../model/document';
import { DocumentKey } from '../model/document_key';
import {
ArrayValue,
FieldValue,
RefValue,
ServerTimestampValue
} from '../model/field_value';
import { DeleteMutation, Mutation, Precondition } from '../model/mutation';
import { FieldPath, ResourcePath } from '../model/path';
import { JsonProtoSerializer } from '../remote/serializer';
import { isServerTimestamp } from '../model/server_timestamps';
import { refValue } from '../model/values';
import { PlatformSupport } from '../platform/platform';
import { makeConstructorPrivate } from '../util/api';
import { assert, fail } from '../util/assert';
Expand Down Expand Up @@ -555,7 +554,10 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
return value;
}
};
return new UserDataReader(preConverter);
const serializer = new JsonProtoSerializer(databaseId, {
useProto3Json: PlatformSupport.getPlatform().useProto3Json
});
return new UserDataReader(serializer, preConverter);
}

private static databaseIdFromApp(app: FirebaseApp): DatabaseId {
Expand Down Expand Up @@ -1390,7 +1392,7 @@ export class DocumentSnapshot<T = firestore.DocumentData>
options.serverTimestamps,
/* converter= */ undefined
);
return userDataWriter.convertValue(this._document.data()) as T;
return userDataWriter.convertValue(this._document.toProto()) as T;
}
}
}
Expand Down Expand Up @@ -1495,7 +1497,7 @@ export class Query<T = firestore.DocumentData> implements firestore.Query<T> {
];
validateStringEnum('Query.where', whereFilterOpEnums, 2, opStr);

let fieldValue: FieldValue;
let fieldValue: api.Value;
const fieldPath = fieldPathFromArgument('Query.where', field);
const operator = Operator.fromString(opStr);
if (fieldPath.isKeyField()) {
Expand All @@ -1510,11 +1512,11 @@ export class Query<T = firestore.DocumentData> implements firestore.Query<T> {
);
} else if (operator === Operator.IN) {
this.validateDisjunctiveFilterElements(value, operator);
const referenceList: FieldValue[] = [];
for (const arrayValue of value as FieldValue[]) {
const referenceList: api.Value[] = [];
for (const arrayValue of value as api.Value[]) {
referenceList.push(this.parseDocumentIdValue(arrayValue));
}
fieldValue = new ArrayValue(referenceList);
fieldValue = { arrayValue: { values: referenceList } };
} else {
fieldValue = this.parseDocumentIdValue(value);
}
Expand Down Expand Up @@ -1720,7 +1722,7 @@ export class Query<T = firestore.DocumentData> implements firestore.Query<T> {
`${methodName}().`
);
}
return this.boundFromDocument(methodName, snap._document!, before);
return this.boundFromDocument(snap._document!, before);
} else {
const allFields = [docOrField].concat(fields);
return this.boundFromFields(methodName, allFields, before);
Expand All @@ -1738,12 +1740,8 @@ export class Query<T = firestore.DocumentData> implements firestore.Query<T> {
* of the query or if any of the fields in the order by are an uncommitted
* server timestamp.
*/
private boundFromDocument(
methodName: string,
doc: Document,
before: boolean
): Bound {
const components: FieldValue[] = [];
private boundFromDocument(doc: Document, before: boolean): Bound {
const components: api.Value[] = [];

// Because people expect to continue/end a query at the exact document
// provided, we need to use the implicit sort order rather than the explicit
Expand All @@ -1754,10 +1752,10 @@ export class Query<T = firestore.DocumentData> implements firestore.Query<T> {
// results.
for (const orderBy of this._query.orderBy) {
if (orderBy.field.isKeyField()) {
components.push(new RefValue(this.firestore._databaseId, doc.key));
components.push(refValue(this.firestore._databaseId, doc.key));
} else {
const value = doc.field(orderBy.field);
if (value instanceof ServerTimestampValue) {
if (isServerTimestamp(value)) {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
'Invalid query. You are trying to start or end a query using a ' +
Expand Down Expand Up @@ -1801,7 +1799,7 @@ export class Query<T = firestore.DocumentData> implements firestore.Query<T> {
);
}

const components: FieldValue[] = [];
const components: api.Value[] = [];
for (let i = 0; i < values.length; i++) {
const rawValue = values[i];
const orderByComponent = orderBy[i];
Expand Down Expand Up @@ -1835,7 +1833,7 @@ export class Query<T = firestore.DocumentData> implements firestore.Query<T> {
);
}
const key = new DocumentKey(path);
components.push(new RefValue(this.firestore._databaseId, key));
components.push(refValue(this.firestore._databaseId, key));
} else {
const wrapped = this.firestore._dataReader.parseQueryValue(
methodName,
Expand Down Expand Up @@ -2034,7 +2032,7 @@ export class Query<T = firestore.DocumentData> implements firestore.Query<T> {
* appropriate errors if the value is anything other than a DocumentReference
* or String, or if the string is malformed.
*/
private parseDocumentIdValue(documentIdValue: unknown): RefValue {
private parseDocumentIdValue(documentIdValue: unknown): api.Value {
if (typeof documentIdValue === 'string') {
if (documentIdValue === '') {
throw new FirestoreError(
Expand Down Expand Up @@ -2065,10 +2063,10 @@ export class Query<T = firestore.DocumentData> implements firestore.Query<T> {
`but '${path}' is not because it has an odd number of segments (${path.length}).`
);
}
return new RefValue(this.firestore._databaseId, new DocumentKey(path));
return refValue(this.firestore._databaseId, new DocumentKey(path));
} else if (documentIdValue instanceof DocumentReference) {
const ref = documentIdValue as DocumentReference<T>;
return new RefValue(this.firestore._databaseId, ref._key);
return refValue(this.firestore._databaseId, ref._key);
} else {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/src/api/field_value.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2017 Google Inc.
* Copyright 2017 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down
Loading