Remove timestampsInSnapshots from FirestoreSettings (#3897)

firebase · Oct 15, 2020 · ffef32e · ffef32e
1 parent f355439
commit ffef32e
Show file tree

Hide file tree

Showing 10 changed files with 15 additions and 163 deletions.
diff --git a/.changeset/strange-rabbits-wait.md 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
@@ -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
@@ -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
@@ -236,7 +236,6 @@ export class DocumentSnapshot<T = DocumentData> 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<T = DocumentData> 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
@@ -171,7 +171,6 @@ export class DocumentSnapshot<T = DocumentData> {
     } else {
       const userDataWriter = new UserDataWriter(
         this._firestore._databaseId,
-        /* timestampsInSnapshots= */ true,
         /* serverTimestampBehavior=*/ 'none',
         key =>
           new DocumentReference(
@@ -204,7 +203,6 @@ export class DocumentSnapshot<T = DocumentData> {
       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
@@ -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,35 +240,13 @@ class FirestoreSettings {
     );
     this.credentials = settings.credentials;
 
-    validateNamedOptionalType(
-      'settings',
-      'boolean',
-      'timestampsInSnapshots',
-      settings.timestampsInSnapshots
-    );
-
     validateNamedOptionalType(
       'settings',
       'boolean',
       'ignoreUndefinedProperties',
       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<T = DocumentData>
       } 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<T = DocumentData>
       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
@@ -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<DocumentData> {

diff --git 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
@@ -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
@@ -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)