From ffef32e3837d3ee1098129b237e7a6e2e738182d Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Thu, 15 Oct 2020 16:06:35 -0500 Subject: [PATCH] Remove `timestampsInSnapshots` from FirestoreSettings (#3897) --- .changeset/strange-rabbits-wait.md | 7 ++ packages/firebase/index.d.ts | 34 --------- packages/firestore-types/index.d.ts | 1 - packages/firestore/exp/src/api/snapshot.ts | 2 - packages/firestore/lite/src/api/snapshot.ts | 2 - packages/firestore/src/api/database.ts | 35 --------- .../firestore/src/api/user_data_writer.ts | 13 +--- .../test/integration/api/fields.test.ts | 71 ------------------- .../test/unit/remote/serializer.helper.ts | 12 ++-- packages/firestore/test/util/helpers.ts | 1 - 10 files changed, 15 insertions(+), 163 deletions(-) create mode 100644 .changeset/strange-rabbits-wait.md diff --git a/.changeset/strange-rabbits-wait.md b/.changeset/strange-rabbits-wait.md new file mode 100644 index 00000000000..abd7ac5b703 --- /dev/null +++ b/.changeset/strange-rabbits-wait.md @@ -0,0 +1,7 @@ +--- +'firebase': major +'@firebase/firestore': major +'@firebase/firestore-types': major +--- + +Removed the `timestampsInSnapshots` option from `FirestoreSettings`. Now, Firestore always returns `Timestamp` values for all timestamp values. diff --git a/packages/firebase/index.d.ts b/packages/firebase/index.d.ts index 3dbef9e2c19..17862f58609 100644 --- a/packages/firebase/index.d.ts +++ b/packages/firebase/index.d.ts @@ -7845,40 +7845,6 @@ declare namespace firebase.firestore { /** Whether to use SSL when connecting. */ ssl?: boolean; - /** - * Specifies whether to use `Timestamp` objects for timestamp fields in - * `DocumentSnapshot`s. This is enabled by default and should not be - * disabled. - * - * Previously, Firestore returned timestamp fields as `Date` but `Date` - * only supports millisecond precision, which leads to truncation and - * causes unexpected behavior when using a timestamp from a snapshot as a - * part of a subsequent query. - * - * Now, Firestore returns `Timestamp` values for all timestamp values stored - * in Cloud Firestore instead of system `Date` objects, avoiding this kind - * of problem. Consequently, you must update your code to handle `Timestamp` - * objects instead of `Date` objects. - * - * If you want to **temporarily** opt into the old behavior of returning - * `Date` objects, you may **temporarily** set `timestampsInSnapshots` to - * false. Opting into this behavior will no longer be possible in the next - * major release of Firestore, after which code that expects Date objects - * **will break**. - * - * @example **Using Date objects in Firestore.** - * // With deprecated setting `timestampsInSnapshot: true`: - * const date : Date = snapshot.get('created_at'); - * // With new default behavior: - * const timestamp : Timestamp = snapshot.get('created_at'); - * const date : Date = timestamp.toDate(); - * - * @deprecated This setting will be removed in a future release. You should - * update your code to expect `Timestamp` objects and stop using the - * `timestampsInSnapshots` setting. - */ - timestampsInSnapshots?: boolean; - /** * An approximate cache size threshold for the on-disk data. If the cache grows beyond this * size, Firestore will start removing data that hasn't been recently used. The size is not a diff --git a/packages/firestore-types/index.d.ts b/packages/firestore-types/index.d.ts index 0222425dea3..bec92576778 100644 --- a/packages/firestore-types/index.d.ts +++ b/packages/firestore-types/index.d.ts @@ -26,7 +26,6 @@ export const CACHE_SIZE_UNLIMITED: number; export interface Settings { host?: string; ssl?: boolean; - timestampsInSnapshots?: boolean; cacheSizeBytes?: number; experimentalForceLongPolling?: boolean; experimentalAutoDetectLongPolling?: boolean; diff --git a/packages/firestore/exp/src/api/snapshot.ts b/packages/firestore/exp/src/api/snapshot.ts index c2cedac1b51..7002d725b39 100644 --- a/packages/firestore/exp/src/api/snapshot.ts +++ b/packages/firestore/exp/src/api/snapshot.ts @@ -236,7 +236,6 @@ export class DocumentSnapshot extends LiteDocumentSnapshot< } else { const userDataWriter = new UserDataWriter( this._firestoreImpl._databaseId, - /* timestampsInSnapshots= */ true, options?.serverTimestamps || DEFAULT_SERVER_TIMESTAMP_BEHAVIOR, key => new DocumentReference( @@ -276,7 +275,6 @@ export class DocumentSnapshot extends LiteDocumentSnapshot< if (value !== null) { const userDataWriter = new UserDataWriter( this._firestoreImpl._databaseId, - /* timestampsInSnapshots= */ true, options.serverTimestamps || DEFAULT_SERVER_TIMESTAMP_BEHAVIOR, key => new DocumentReference(this._firestore, this._converter, key.path), diff --git a/packages/firestore/lite/src/api/snapshot.ts b/packages/firestore/lite/src/api/snapshot.ts index 0b3f9582ad6..cff40b44c0b 100644 --- a/packages/firestore/lite/src/api/snapshot.ts +++ b/packages/firestore/lite/src/api/snapshot.ts @@ -171,7 +171,6 @@ export class DocumentSnapshot { } else { const userDataWriter = new UserDataWriter( this._firestore._databaseId, - /* timestampsInSnapshots= */ true, /* serverTimestampBehavior=*/ 'none', key => new DocumentReference( @@ -204,7 +203,6 @@ export class DocumentSnapshot { if (value !== null) { const userDataWriter = new UserDataWriter( this._firestore._databaseId, - /* timestampsInSnapshots= */ true, /* serverTimestampBehavior=*/ 'none', key => new DocumentReference(this._firestore, this._converter, key.path), diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index 492338adb95..a1d835afd8f 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -151,7 +151,6 @@ import { // settings() defaults: const DEFAULT_HOST = 'firestore.googleapis.com'; const DEFAULT_SSL = true; -const DEFAULT_TIMESTAMPS_IN_SNAPSHOTS = true; const DEFAULT_FORCE_LONG_POLLING = false; const DEFAULT_FORCE_AUTO_DETECT_LONG_POLLING = false; const DEFAULT_IGNORE_UNDEFINED_PROPERTIES = false; @@ -194,8 +193,6 @@ class FirestoreSettings { /** Whether to use SSL when connecting. */ readonly ssl: boolean; - readonly timestampsInSnapshots: boolean; - readonly cacheSizeBytes: number; readonly experimentalForceLongPolling: boolean; @@ -229,7 +226,6 @@ class FirestoreSettings { 'host', 'ssl', 'credentials', - 'timestampsInSnapshots', 'cacheSizeBytes', 'experimentalForceLongPolling', 'experimentalAutoDetectLongPolling', @@ -244,13 +240,6 @@ class FirestoreSettings { ); this.credentials = settings.credentials; - validateNamedOptionalType( - 'settings', - 'boolean', - 'timestampsInSnapshots', - settings.timestampsInSnapshots - ); - validateNamedOptionalType( 'settings', 'boolean', @@ -258,21 +247,6 @@ class FirestoreSettings { settings.ignoreUndefinedProperties ); - // Nobody should set timestampsInSnapshots anymore, but the error depends on - // whether they set it to true or false... - if (settings.timestampsInSnapshots === true) { - logError( - "The setting 'timestampsInSnapshots: true' is no longer required " + - 'and should be removed.' - ); - } else if (settings.timestampsInSnapshots === false) { - logError( - "Support for 'timestampsInSnapshots: false' will be removed soon. " + - 'You must update your code to handle Timestamp objects.' - ); - } - this.timestampsInSnapshots = - settings.timestampsInSnapshots ?? DEFAULT_TIMESTAMPS_IN_SNAPSHOTS; this.ignoreUndefinedProperties = settings.ignoreUndefinedProperties ?? DEFAULT_IGNORE_UNDEFINED_PROPERTIES; @@ -329,7 +303,6 @@ class FirestoreSettings { return ( this.host === other.host && this.ssl === other.ssl && - this.timestampsInSnapshots === other.timestampsInSnapshots && this.credentials === other.credentials && this.cacheSizeBytes === other.cacheSizeBytes && this.experimentalForceLongPolling === @@ -810,12 +783,6 @@ export class Firestore implements PublicFirestore, FirebaseService { setLogLevel(level); } - // Note: this is not a property because the minifier can't work correctly with - // the way TypeScript compiler outputs properties. - _areTimestampsInSnapshotsEnabled(): boolean { - return this._settings.timestampsInSnapshots; - } - // Visible for testing. _getSettings(): PublicSettings { return this._settings; @@ -1498,7 +1465,6 @@ export class DocumentSnapshot } else { const userDataWriter = new UserDataWriter( this._firestore._databaseId, - this._firestore._areTimestampsInSnapshotsEnabled(), options.serverTimestamps || 'none', key => new DocumentReference(key, this._firestore, /* converter= */ null), @@ -1524,7 +1490,6 @@ export class DocumentSnapshot if (value !== null) { const userDataWriter = new UserDataWriter( this._firestore._databaseId, - this._firestore._areTimestampsInSnapshotsEnabled(), options.serverTimestamps || 'none', key => new DocumentReference(key, this._firestore, this._converter), bytes => new Blob(bytes) diff --git a/packages/firestore/src/api/user_data_writer.ts b/packages/firestore/src/api/user_data_writer.ts index f36f9244a53..84888fe6fd2 100644 --- a/packages/firestore/src/api/user_data_writer.ts +++ b/packages/firestore/src/api/user_data_writer.ts @@ -57,7 +57,6 @@ export type ServerTimestampBehavior = 'estimate' | 'previous' | 'none'; export class UserDataWriter { constructor( private readonly databaseId: DatabaseId, - private readonly timestampsInSnapshots: boolean, private readonly serverTimestampBehavior: ServerTimestampBehavior, private readonly referenceFactory: ( key: DocumentKey @@ -128,17 +127,9 @@ export class UserDataWriter { } } - private convertTimestamp(value: ProtoTimestamp): Timestamp | Date { + private convertTimestamp(value: ProtoTimestamp): Timestamp { const normalizedValue = normalizeTimestamp(value); - const timestamp = new Timestamp( - normalizedValue.seconds, - normalizedValue.nanos - ); - if (this.timestampsInSnapshots) { - return timestamp; - } else { - return timestamp.toDate(); - } + return new Timestamp(normalizedValue.seconds, normalizedValue.nanos); } private convertReference(name: string): _DocumentKeyReference { diff --git a/packages/firestore/test/integration/api/fields.test.ts b/packages/firestore/test/integration/api/fields.test.ts index 5d3b7f09b34..6debcac6bc8 100644 --- a/packages/firestore/test/integration/api/fields.test.ts +++ b/packages/firestore/test/integration/api/fields.test.ts @@ -28,9 +28,7 @@ import { import { DEFAULT_SETTINGS } from '../util/settings'; const FieldPath = firebaseExport.FieldPath; -const FieldValue = firebaseExport.FieldValue; const Timestamp = firebaseExport.Timestamp; -const usesFunctionalApi = firebaseExport.usesFunctionalApi; // Allow custom types for testing. // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -354,44 +352,6 @@ apiDescribe('Timestamp Fields in snapshots', (persistence: boolean) => { return { timestamp: ts, nested: { timestamp2: ts } }; }; - // timestampInSnapshots is not support in the modular API. - // eslint-disable-next-line no-restricted-properties - (usesFunctionalApi() ? it.skip : it)( - 'are returned as native dates if timestampsInSnapshots set to false', - () => { - const settings = { ...DEFAULT_SETTINGS }; - settings['timestampsInSnapshots'] = false; - - const timestamp = new Timestamp(100, 123456789); - const testDocs = { a: testDataWithTimestamps(timestamp) }; - return withTestCollectionSettings( - persistence, - settings, - testDocs, - coll => { - return coll - .doc('a') - .get() - .then(docSnap => { - expect(docSnap.get('timestamp')) - .to.be.a('date') - .that.deep.equals(timestamp.toDate()); - expect(docSnap.data()!['timestamp']) - .to.be.a('date') - .that.deep.equals(timestamp.toDate()); - - expect(docSnap.get('nested.timestamp2')) - .to.be.a('date') - .that.deep.equals(timestamp.toDate()); - expect(docSnap.data()!['nested']['timestamp2']) - .to.be.a('date') - .that.deep.equals(timestamp.toDate()); - }); - } - ); - } - ); - it('are returned as Timestamps', () => { const timestamp = new Timestamp(100, 123456000); // Timestamps are currently truncated to microseconds after being written to @@ -423,37 +383,6 @@ apiDescribe('Timestamp Fields in snapshots', (persistence: boolean) => { }); }); }); - - // timestampInSnapshots is not support in the modular API. - // eslint-disable-next-line no-restricted-properties - (usesFunctionalApi() ? it.skip : it)( - 'timestampsInSnapshots affects server timestamps', - () => { - const settings = { ...DEFAULT_SETTINGS }; - settings['timestampsInSnapshots'] = false; - const testDocs = { - a: { timestamp: FieldValue.serverTimestamp() } - }; - - return withTestCollectionSettings( - persistence, - settings, - testDocs, - coll => { - return coll - .doc('a') - .get() - .then(docSnap => { - expect( - docSnap.get('timestamp', { - serverTimestamps: 'estimate' - }) - ).to.be.an.instanceof(Date); - }); - } - ); - } - ); }); apiDescribe('`undefined` properties', (persistence: boolean) => { diff --git a/packages/firestore/test/unit/remote/serializer.helper.ts b/packages/firestore/test/unit/remote/serializer.helper.ts index 5de62d045ca..2711be84c2a 100644 --- a/packages/firestore/test/unit/remote/serializer.helper.ts +++ b/packages/firestore/test/unit/remote/serializer.helper.ts @@ -310,8 +310,8 @@ export function serializerTest( it('converts TimestampValue from proto', () => { const examples = [ - new Date(Date.UTC(2016, 0, 2, 10, 20, 50, 850)), - new Date(Date.UTC(2016, 5, 17, 10, 50, 15, 0)) + new Timestamp(1451730050, 850000000), + new Timestamp(1466160615, 0) ]; const expectedJson = [ @@ -339,25 +339,25 @@ export function serializerTest( userDataWriter.convertValue({ timestampValue: '2017-03-07T07:42:58.916123456Z' }) - ).to.deep.equal(new Timestamp(1488872578, 916123456).toDate()); + ).to.deep.equal(new Timestamp(1488872578, 916123456)); expect( userDataWriter.convertValue({ timestampValue: '2017-03-07T07:42:58.916123Z' }) - ).to.deep.equal(new Timestamp(1488872578, 916123000).toDate()); + ).to.deep.equal(new Timestamp(1488872578, 916123000)); expect( userDataWriter.convertValue({ timestampValue: '2017-03-07T07:42:58.916Z' }) - ).to.deep.equal(new Timestamp(1488872578, 916000000).toDate()); + ).to.deep.equal(new Timestamp(1488872578, 916000000)); expect( userDataWriter.convertValue({ timestampValue: '2017-03-07T07:42:58Z' }) - ).to.deep.equal(new Timestamp(1488872578, 0).toDate()); + ).to.deep.equal(new Timestamp(1488872578, 0)); }); it('converts TimestampValue to string (useProto3Json=true)', () => { diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts index ef111de3c56..67c63479b9d 100644 --- a/packages/firestore/test/util/helpers.ts +++ b/packages/firestore/test/util/helpers.ts @@ -111,7 +111,6 @@ export type TestSnapshotVersion = number; export function testUserDataWriter(): UserDataWriter { return new UserDataWriter( TEST_DATABASE_ID, - /* timestampsInSnapshots= */ false, 'none', key => new DocumentReference(key, FIRESTORE, /* converter= */ null), bytes => new Blob(bytes)