Skip to content

Add small Proto helpers #2753

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 17 commits into from
Mar 19, 2020
Merged
Show file tree
Hide file tree
Changes from 10 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
4 changes: 3 additions & 1 deletion packages/firestore/src/api/user_data_writer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ export class UserDataWriter<T> {
private convertServerTimestamp(value: ServerTimestampValue): unknown {
switch (this.serverTimestampBehavior) {
case 'previous':
return value.previousValue ? this.convertValue(value.previousValue) : null;
return value.previousValue
? this.convertValue(value.previousValue)
: null;
case 'estimate':
return this.convertTimestamp(value.localWriteTime);
default:
Expand Down
4 changes: 4 additions & 0 deletions packages/firestore/src/model/document_key.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ export class DocumentKey {
);
}

static fromName(name: string): DocumentKey {
return new DocumentKey(ResourcePath.fromString(name).popFirst(5));
}

/** Returns true if the document is in the specified collectionId. */
hasCollectionId(collectionId: string): boolean {
return (
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/src/model/proto_field_value.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import {
normalizeByteString,
normalizeTimestamp,
typeOrder
} from './proto_values';
} from './values';
import { Blob } from '../api/blob';
import { GeoPoint } from '../api/geo_point';
import { Timestamp } from '../api/timestamp';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { TypeOrder } from './field_value';
import { assert, fail } from '../util/assert';
import { forEach, keys, size } from '../util/obj';
import { ByteString } from '../util/byte_string';
import { DocumentKey } from './document_key';
import {
numericComparator,
numericEquals,
Expand Down Expand Up @@ -359,8 +360,7 @@ function canonifyValue(value: api.Value): string {
} else if ('bytesValue' in value) {
return canonifyByteString(value.bytesValue!);
} else if ('referenceValue' in value) {
// TODO(mrschmidt): Use document key only
return value.referenceValue!;
return canonifyReference(value.referenceValue!);
} else if ('geoPointValue' in value) {
return canonifyGeoPoint(value.geoPointValue!);
} else if ('arrayValue' in value) {
Expand All @@ -385,6 +385,10 @@ function canonifyGeoPoint(geoPoint: api.LatLng): string {
return `geo(${geoPoint.latitude},${geoPoint.longitude})`;
}

function canonifyReference(referenceValue: string): string {
return DocumentKey.fromName(referenceValue).toString();
}

function canonifyMap(mapValue: api.MapValue): string {
// Iteration order in JavaScript is not guaranteed. To ensure that we generate
// matching canonical IDs for identical maps, we need to sort the keys.
Expand Down Expand Up @@ -535,3 +539,43 @@ export function normalizeByteString(blob: string | Uint8Array): ByteString {
return ByteString.fromUint8Array(blob);
}
}

/** Returns true if `value` is an IntegerValue. */
export function isInteger(value?: api.Value | null): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we defined an interface like

exporter interface IntegerValue {
  integerValue: number;
}

Then we could make this a type guard:

export function isInteger(value?: api.Value | null): value is IntegerValue {
  return !!value && 'integerValue' in value;
}

This PR doesn't include any uses of these functions, so it's not clear that this would necessarily be a win, but it seems like this could be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the PR to allow us to make asserts on the input type, but I kept it in the method declarations for now and did not plumb through separate types. This should allow us to get rid of bangs for future usages.

return !!value && 'integerValue' in value;
}

/** Returns true if `value` is a DoubleValue. */
export function isDouble(value?: api.Value | null): boolean {
return !!value && 'doubleValue' in value;
}

/** Returns true if `value` is either an IntegerValue or a DoubleValue. */
export function isNumber(value?: api.Value | null): boolean {
return isInteger(value) || isDouble(value);
}

/** Returns true if `value` is an ArrayValue. */
export function isArray(value?: api.Value | null): boolean {
return !!value && 'arrayValue' in value;
}

/** Returns true if `value` is a ReferenceValue. */
export function isReferenceValue(value?: api.Value | null): boolean {
return !!value && 'referenceValue' in value;
}

/** Returns true if `value` is a NullValue. */
export function isNullValue(value?: api.Value | null): boolean {
return !!value && 'nullValue' in value;
}

/** Returns true if `value` is NaN. */
export function isNanValue(value?: api.Value | null): boolean {
return !!value && isNaN(Number(value.doubleValue));
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't actually test that value is a double. Should this be isDouble(value) && isNaN(Number(value.doubleValue));?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh JavaScript:

isNaN(Number(null)) => false
isNaN(Number(undefined)) => true

So it looks like we need to test here that value.doubleValue is set.

}

/** Returns true if `value` is a MapValue. */
export function isMapValue(value?: api.Value | null): boolean {
return !!value && 'mapValue' in value;
}
2 changes: 1 addition & 1 deletion packages/firestore/src/remote/serializer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ import {
normalizeByteString,
normalizeNumber,
normalizeTimestamp
} from '../model/proto_values';
} from '../model/values';

const DIRECTIONS = (() => {
const dirs: { [dir: string]: api.OrderDirection } = {};
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/src/util/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export function isNullOrUndefined(value: unknown): boolean {
}

/** Returns whether the value represents -0. */
export function isNegativeZero(value: number) : boolean {
export function isNegativeZero(value: number): boolean {
// Detect if the value is -0.0. Based on polyfill from
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is
return value === -0 && 1 / value === 1 / -0;
Expand Down
Loading