Make Timestamp a part of the public Firestore API. #544

var-const · 2018-03-03T00:49:23Z

No description provided.

This is a transitional state; eventually, Timestamp will live in a new separate package (common-types/). Also: - move Timestamp.fromISOString method into Serializer, the only place of usage; - rename nanos to nanoseconds.

Truncate Timestamps to microseconds immediately upon receiving them as a parameter, so that offline and online behavior is consistent.

The behavior to return timestamp fields as dates is now legacy. It's on by default to allow for a transitional period. The new behavior to return Timestamp objects instead is opt-in, but will become default in the future. The advantage Timestamps have is higher precision than native browser Date. If the new behavior is not enabled, a warning is logged on startup urging users to upgrade.

var-const

Hi Michael,

I decided to do this as a single PR, partially because it's similar to other platforms that were reviewed already, and partially because line count is lower in TypeScript. The PR is split into 3 meaningful commits. If you'd rather have me split this into 3 PRs, I'll do it.

The one thing I'm not sure about is the suggestion from API review that Timestamp should live in a common-types/ folder. Currently, all such folders come in pairs, e.g. firestore/ and firestore-types/. I'm not sure if it should be common/ and common-types/?

var-const · 2018-03-03T00:50:51Z

packages/firestore-types/index.d.ts

+   * Creates a new timestamp from the given number of milliseconds since Unix
+   * epoch 1970-01-01T00:00:00Z.
+   */
+  static fromMillis(milliseconds: number): Timestamp;


I changed the name to match the API review doc, let me know if you think we should try to push back on this.

var-const · 2018-03-03T00:52:30Z

packages/firestore/src/api/database.ts

@@ -296,6 +321,32 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
      'FirestoreSettings.host cannot be falsey'
    );

+    if (!this._config.settings.timestampsInSnapshotsEnabled) {
+      log.error(`
+The default behavior for Date objects stored in Firestore is going to change


After seeing this message in the logs, wrapping it to 80 columns seemed like a good idea.

This message gets logged about a thousand times during tests. It's super-annoying, do you have any suggestions on how to disable it during a test run?

Good call.

Can we just set it to true, at least for the majority of our tests? That would be desirable, since that'll be the eventual default anyway.

FWIW- since this is javascript, we could have tests do hackiness like:

let oldConsoleError = console.error; console.error = () => { }; ... console.error = oldConsoleError;

That might be okay if there was a very well-defined scope where we wanted to suppress the log message, but it's probably not a great solution. :-)

Do you think this applies to other platforms as well? I'm not sure how developers usually read logs there.

Setting it to true sounds good, but I'd like to clarify before implementing: it leads to breakage of 11 existing tests, mostly around server timestamps. Do you think the tests should be changed to anticipate the new behavior, or should I specifically use the old behavior in these tests?

Hrm. I suspect wrapping to 80 columns would be friendly on the other platforms, though I don't remember if Xcode and Android Studio wrap automatically in their consoles or not. So when you revisit the log messages pending API review feedback, etc., maybe wrap them, but I am not too concerned about it.

I think the 11 tests should be changed to expect the new behavior, but I'd make sure we have at least one test that verifies server timestamps return Dates (for ServerTimestampBehavior 'estimate') if you have the enableTimestampsInSnapshots setting turned off.

Changed the tests, including one test for server timestamps that previously checked for Timestamps if the setting is true, but now checks for Dates if setting is false.

var-const · 2018-03-03T00:53:16Z

packages/firestore/src/api/database.ts

@@ -387,7 +438,9 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
      if (this._firestoreClient) {
        return this._firestoreClient.shutdown();
      }
-    }
+    },


I'm not sure if this should be in INTERNAL. My reasoning was that it seems that in TypeScript, current settings aren't exposed (or I just missed it).

INTERNAL is semi-public. So as-is users of the SDK could access this via firebase.firestore().INTERNAL.areTimestampsInSnapshotsEnabled() which we don't really want. So I think instead I'd make this a property on Firestore:

get _areTimestampsInSnapshotsEnabled(): boolean {
return this._config.settings.timestampsInSnapshotsEnabled;
}

Note that we use '_' for any members of our public API classes (like Firestore) that we don't want to be visible in our public API. There's a minification step as part of our build that will rename '_blah' properties to obfuscated names.

Done, thanks for the explanation.

var-const · 2018-03-03T00:54:09Z

packages/firestore/src/api/timestamp.ts

+
+  constructor(readonly seconds: number, readonly nanoseconds: number) {
+    if (nanoseconds < 0) {
+      throw new FirestoreError(


Use throw instead of assert.

I'm not sure if this comment is asking a question, but I think throw is appropriate since this is a user-facing error rather than an internal assertion.

Sorry for not being clear, I just wanted to draw attention to the change.

var-const · 2018-03-03T00:55:06Z

packages/firestore/src/model/field_value.ts

@@ -69,21 +69,32 @@ export enum ServerTimestampBehavior {

 /** Holds properties that define field value deserialization options. */
 export class FieldValueOptions {
-  static readonly defaultOptions = new FieldValueOptions(


Removed defaultOptions like elsewhere.

var-const · 2018-03-03T00:56:36Z

packages/firestore/src/remote/serializer.ts

@@ -83,6 +83,9 @@ const OPERATORS = (() => {
  return ops;
 })();

+// A RegExp matching ISO 8601 UTC timestamps with optional fraction.
+const ISO_REG_EXP = new RegExp(/^\d{4}-\d\d-\d\dT\d\d:\d\d:\d\d(?:\.(\d+))?Z$/);


I originally moved everything connected to parsing ISO strings here to avoid exposing this functionality in the public API. I guess it was possible to leave it on the Timestamp and not declare it in index.d.ts file (at least I assume so), but on the other hand, that functionality is only used in the serializer. Feel free to push back on this.

Yeah, it would be fine to have it in Timestamp but not expose it in the .d.ts. Though we would also want to prefix the APIs with '_' to ensure they're minified. That said, it seems like ISO timestamp conversion is specific to the web serializer implementation (we don't have iso timestamp conversion in the iOS / Android Timestamp.java implementation) and so keeping it in the serializer sounds good to me.

var-const · 2018-03-03T00:57:46Z

packages/firestore/test/integration/util/helpers.ts

@@ -86,6 +86,11 @@ export function toDataArray(
  return docSet.docs.map(d => d.data());
 }

+/** Converts a DocumentSet to an array with the id of each document */
+export function toIds(docSet: firestore.QuerySnapshot): String[] {


There is a similar helper function on iOS and Android (I hope I didn't miss it here).

It's possible it was omitted because map() is so easy in JS, but I'm not opposed to having it.

var-const · 2018-03-03T00:58:04Z

packages/firestore/test/unit/api/timestamp.test.ts

-    });
-  });
-
-  it('fromISOString', () => {


Moved to serializer tests.

mikelehen

Awesome. I haven't reviewed the code yet but I preemptively replied to a couple of your comments. As for where to put Timestamp (creating a common/ and common-types/ directory, etc.), that is a question for @jshcrowthe. Josh, is this on your radar? We've been asked to contribute Timestamp as a "common" type to be used by Firestore but potentially other parts of the Firebase SDK in the future (ping me or @var-const for more details).

mikelehen · 2018-03-03T01:13:15Z

packages/firestore/src/api/database.ts

@@ -387,7 +438,9 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
      if (this._firestoreClient) {
        return this._firestoreClient.shutdown();
      }
-    }
+    },


INTERNAL is semi-public. So as-is users of the SDK could access this via firebase.firestore().INTERNAL.areTimestampsInSnapshotsEnabled() which we don't really want. So I think instead I'd make this a property on Firestore:

get _areTimestampsInSnapshotsEnabled(): boolean {
return this._config.settings.timestampsInSnapshotsEnabled;
}

Note that we use '_' for any members of our public API classes (like Firestore) that we don't want to be visible in our public API. There's a minification step as part of our build that will rename '_blah' properties to obfuscated names.

mikelehen · 2018-03-03T01:14:14Z

packages/firestore/src/api/timestamp.ts

+
+  constructor(readonly seconds: number, readonly nanoseconds: number) {
+    if (nanoseconds < 0) {
+      throw new FirestoreError(


I'm not sure if this comment is asking a question, but I think throw is appropriate since this is a user-facing error rather than an internal assertion.

mikelehen

Overall looks great. Have a few minor requests, mostly around comments, etc.

mikelehen · 2018-03-05T22:55:42Z

packages/firestore-types/index.d.ts

+   * DocumentSnapshot}s.
+   *
+   * Currently, Firestore returns timestamp fields as {@link Date} but {@link
+   * Date} only supports millisecond precision, which leads to truncation and


Interesting! I didn't know @link was available in JSDoc / TypeDoc (this is TypeScript so in theory we'd use something like TypeDoc, but in practice we end up copying these comments into an externs file and generating docs via JSDoc :-/). That said, I'd be surprised if {@link Date} will do anything useful, since Date isn't defined in the codebase (and there isn't a canonical location for docs on JavaScript standard types). So I'd be inclined to do Date or "a JavaScript Date object" or similar.

Examining the file, it appears that class names are just wrapped in code tags ``. Changed all usages of @link to this. What do you think about using Date and wrapping it in backticks?

Yeah, that seems fine and consistent with what we've been doing. Using @link might be a good idea, but we can tackle that holistically at a later time.

mikelehen · 2018-03-05T22:58:39Z

packages/firestore-types/index.d.ts

@@ -20,21 +20,45 @@ import { FirebaseApp, FirebaseNamespace } from '@firebase/app-types';
 * Document data (for use with `DocumentReference.set()`) consists of fields
 * mapped to values.
 */
-export type DocumentData = { [field: string]: any };
+export type DocumentData = {
+  [field: string]: any;


Did you intentionally add these semicolons? I don't care one way or the other. I'm surprised that it's apparently optional.

I'm pretty sure this is from the precommit code formatting. I'll have to figure out why it reformats so much before merging; rerunning yarn didn't seem to help.

Oh, I see. It looks like if I remove the semicolon, prettier adds it back, but if I completely undo this change (remove the semicolon and put this back to being one line) then prettier leaves it as-is... Maybe you ended up formatting with a "wrong" version and then the "right" version, which had a different net effect than just formatting with the "right" version or something? I don't know... Anyway, I'm not inclined to worry about it for this PR...

Hmm, this sounds like it could be the cause. I first formatted using clang-format, which resulted in a pretty different formatting, and I guess prettier couldn't restore it successfully.

mikelehen · 2018-03-05T23:06:01Z

packages/firestore-types/index.d.ts

+   */
+  static now(): Timestamp;
+
+  /** Creates a new timestamp from the given date. */


Can you add the @param / @return comments? I'm happy to omit them in our normal codebase, but this .d.ts file (indirectly) ends up being used to generate docs, so I think we want to be complete.

Done.

These comments become pretty repetitive, please let me know what you think.

mikelehen · 2018-03-05T23:11:13Z

packages/firestore-types/index.d.ts

+  /**
+   * Creates a new timestamp.
+   *
+   * @param seconds represents seconds of UTC time since Unix epoch


I think rather than "seconds represents seconds ..." we probably want "seconds The number of seconds ..." or similar to match our other comments.

mikelehen · 2018-03-06T00:40:19Z

packages/firestore-types/index.d.ts

+   */
+  toMillis(): number;
+
+  compareTo(other: Timestamp): number;


I don't think we should expose compareTo() in our public API. We don't on any of our other types today.

mikelehen · 2018-03-06T01:44:10Z

packages/firestore/test/integration/util/helpers.ts

@@ -86,6 +86,11 @@ export function toDataArray(
  return docSet.docs.map(d => d.data());
 }

+/** Converts a DocumentSet to an array with the id of each document */
+export function toIds(docSet: firestore.QuerySnapshot): String[] {


It's possible it was omitted because map() is so easy in JS, but I'm not opposed to having it.

mikelehen · 2018-03-06T01:46:04Z

packages/firestore/test/integration/api/cursor.test.ts

+  const makeTimestamp = (seconds, micros) =>
+    new Timestamp(seconds, micros * 1000);
+
+  it('can accept Timestamps as limits', () => {


Not sure if this was copied from elsewhere, but since we have a limit() API, this would be clearer as "can accept Timestamps as bounds" or similar.

Done. Yes, I used similar test names on other platforms, I guess I'll change them to refer to "bounds" as well.

mikelehen · 2018-03-06T01:49:13Z

packages/firestore/test/unit/remote/node/serializer.test.ts

-          longitude: 4.56
-        }
-      };
+      const expected = { geoPointValue: { latitude: 1.23, longitude: 4.56 } };


I'm curious if you reformatted these or prettier did it for some reason...

prettier is the culprit!

mikelehen · 2018-03-06T01:51:18Z

packages/firestore/src/api/timestamp.ts

+    return this.seconds * 1000 + this.nanoseconds / 1e6;
+  }
+
+  compareTo(other: Timestamp): number {


Assuming we don't make this public, can you rename to _compareTo()?

Done.

Does it apply to isEqual as well?

isEqual() is considered part of our public API and so we don't want it minified and so it should stay isEqual(), not _isEqual().

mikelehen · 2018-03-06T01:52:01Z

packages/firestore-types/index.d.ts

+
+  isEqual(other: Timestamp): boolean;
+
+  toString(): string;


toString() exists on any Object in JavaScript and doesn't need to be explicitly listed here.

var-const

One more question: it looks like dist/ folder contains build artifacts. Should I update it? If yes, how do I do it?

var-const · 2018-03-08T19:18:44Z

packages/firestore-types/index.d.ts

@@ -20,21 +20,45 @@ import { FirebaseApp, FirebaseNamespace } from '@firebase/app-types';
 * Document data (for use with `DocumentReference.set()`) consists of fields
 * mapped to values.
 */
-export type DocumentData = { [field: string]: any };
+export type DocumentData = {
+  [field: string]: any;


I'm pretty sure this is from the precommit code formatting. I'll have to figure out why it reformats so much before merging; rerunning yarn didn't seem to help.

var-const · 2018-03-08T19:19:00Z

packages/firestore-types/index.d.ts

+   * DocumentSnapshot}s.
+   *
+   * Currently, Firestore returns timestamp fields as {@link Date} but {@link
+   * Date} only supports millisecond precision, which leads to truncation and


Examining the file, it appears that class names are just wrapped in code tags ``. Changed all usages of @link to this. What do you think about using Date and wrapping it in backticks?

var-const · 2018-03-08T19:25:36Z

packages/firestore/src/api/database.ts

@@ -127,6 +128,8 @@ class FirestoreSettings {
  /** Whether to use SSL when connecting. */
  ssl: boolean;

+  timestampsInSnapshotsEnabled: boolean;


var-const · 2018-03-08T19:29:44Z

packages/firestore/src/api/database.ts

  }

  isEqual(other: FirestoreSettings): boolean {
    return (
      this.host === other.host &&
      this.ssl === other.ssl &&
+      this.timestampsInSnapshotsEnabled == other.timestampsInSnapshotsEnabled &&


var-const · 2018-03-08T19:31:52Z

packages/firestore/src/api/timestamp.ts

+
+  constructor(readonly seconds: number, readonly nanoseconds: number) {
+    if (nanoseconds < 0) {
+      throw new FirestoreError(


Sorry for not being clear, I just wanted to draw attention to the change.

var-const · 2018-03-08T20:51:49Z

packages/firestore-types/index.d.ts

+   */
+  static now(): Timestamp;
+
+  /** Creates a new timestamp from the given date. */


Done.

These comments become pretty repetitive, please let me know what you think.

var-const · 2018-03-08T20:51:58Z

packages/firestore-types/index.d.ts

+  /**
+   * Creates a new timestamp.
+   *
+   * @param seconds represents seconds of UTC time since Unix epoch


var-const · 2018-03-08T20:52:05Z

packages/firestore-types/index.d.ts

+
+  compareTo(other: Timestamp): number;
+
+  isEqual(other: Timestamp): boolean;


var-const · 2018-03-08T21:01:15Z

packages/firestore/src/api/database.ts

@@ -387,7 +438,9 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
      if (this._firestoreClient) {
        return this._firestoreClient.shutdown();
      }
-    }
+    },


Done, thanks for the explanation.

var-const · 2018-03-08T21:04:44Z

packages/firestore/src/api/database.ts

+        settings.timestampsInSnapshotsEnabled,
+        DEFAULT_TIMESTAMPS_IN_SNAPSHOTS_ENABLED
+      );
+      this.timestampsInSnapshotsEnabled = settings.timestampsInSnapshotsEnabled;


Done, thanks for spotting it.

- add additional comments in Timestamp declarations; - remove unnecessary method declarations from Timestamp; - rename timestampsInSnapshotsEnabled -> timestampsInSnapshots; - use timestampsInSnapshots = true in tests; - small code cleanup.

mikelehen

Thanks! Changes look good pending the remaining test fixup...

mikelehen · 2018-03-09T16:11:07Z

packages/firestore-types/index.d.ts

+   * DocumentSnapshot}s.
+   *
+   * Currently, Firestore returns timestamp fields as {@link Date} but {@link
+   * Date} only supports millisecond precision, which leads to truncation and


Yeah, that seems fine and consistent with what we've been doing. Using @link might be a good idea, but we can tackle that holistically at a later time.

mikelehen · 2018-03-09T16:26:24Z

packages/firestore-types/index.d.ts

@@ -20,21 +20,45 @@ import { FirebaseApp, FirebaseNamespace } from '@firebase/app-types';
 * Document data (for use with `DocumentReference.set()`) consists of fields
 * mapped to values.
 */
-export type DocumentData = { [field: string]: any };
+export type DocumentData = {
+  [field: string]: any;


Oh, I see. It looks like if I remove the semicolon, prettier adds it back, but if I completely undo this change (remove the semicolon and put this back to being one line) then prettier leaves it as-is... Maybe you ended up formatting with a "wrong" version and then the "right" version, which had a different net effect than just formatting with the "right" version or something? I don't know... Anyway, I'm not inclined to worry about it for this PR...

mikelehen · 2018-03-09T16:31:08Z

packages/firestore/src/api/database.ts

@@ -296,6 +321,32 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
      'FirestoreSettings.host cannot be falsey'
    );

+    if (!this._config.settings.timestampsInSnapshotsEnabled) {
+      log.error(`
+The default behavior for Date objects stored in Firestore is going to change


Hrm. I suspect wrapping to 80 columns would be friendly on the other platforms, though I don't remember if Xcode and Android Studio wrap automatically in their consoles or not. So when you revisit the log messages pending API review feedback, etc., maybe wrap them, but I am not too concerned about it.

I think the 11 tests should be changed to expect the new behavior, but I'd make sure we have at least one test that verifies server timestamps return Dates (for ServerTimestampBehavior 'estimate') if you have the enableTimestampsInSnapshots setting turned off.

mikelehen · 2018-03-09T16:37:25Z

packages/firestore/src/api/timestamp.ts

+    return this.seconds * 1000 + this.nanoseconds / 1e6;
+  }
+
+  compareTo(other: Timestamp): number {


isEqual() is considered part of our public API and so we don't want it minified and so it should stay isEqual(), not _isEqual().

Also add a test to make sure server timestamps retain the old behavior if the global settings if not set.

var-const

I will hold off merging until API review passes. Also, I have a question: it looks like dist/ folder contains build artifacts, do I have to/how do I update its contents?

var-const · 2018-03-09T23:04:01Z

packages/firestore/src/api/database.ts

@@ -157,12 +165,29 @@ class FirestoreSettings {
      settings.credentials
    );
    this.credentials = settings.credentials;
+
+    if (settings.timestampsInSnapshotsEnabled === undefined) {
+      this.timestampsInSnapshotsEnabled = DEFAULT_TIMESTAMPS_IN_SNAPSHOTS_ENABLED;


var-const · 2018-03-09T23:05:07Z

packages/firestore/src/api/database.ts

@@ -296,6 +321,32 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
      'FirestoreSettings.host cannot be falsey'
    );

+    if (!this._config.settings.timestampsInSnapshotsEnabled) {
+      log.error(`
+The default behavior for Date objects stored in Firestore is going to change


Changed the tests, including one test for server timestamps that previously checked for Timestamps if the setting is true, but now checks for Dates if setting is false.

var-const · 2018-03-09T23:06:16Z

packages/firestore-types/index.d.ts

@@ -20,21 +20,45 @@ import { FirebaseApp, FirebaseNamespace } from '@firebase/app-types';
 * Document data (for use with `DocumentReference.set()`) consists of fields
 * mapped to values.
 */
-export type DocumentData = { [field: string]: any };
+export type DocumentData = {
+  [field: string]: any;


Hmm, this sounds like it could be the cause. I first formatted using clang-format, which resulted in a pretty different formatting, and I guess prettier couldn't restore it successfully.

mikelehen

Test changes look good. Thanks! Also, I forgot to reply to your comment about dist/ but you shouldn't ever need to check in anything there. That's all build output, and shouldn't be in git AFAIK.

var-const

@mikelehen Michael, sorry, I have a couple more questions:

I just noticed that the automated "prettier" commit added a huge package-lock.json file. I'm pretty sure this has happened before, but I didn't notice and didn't remove it this time. Is this a known issue?
since in this PR, we decided to configure tests to use the new behavior, do you think other platforms should be updated to do the same?

mikelehen · 2018-03-10T00:26:05Z

Hrm. My guess is you accidentally created the package-lock.json file while the prepush hook was running and it looks like (

firebase-js-sdk/tools/gitHooks/prettier.js

Line 50 in 69c9a97

await git.add('.');

) it just does a "git add ." at the end and so it inadvertently included it... cc/ @jshcrowthe We should probably do "git add -u" or skip "git add" and just do a "git commit -a"

I'm not 100% sure what you would have done to create package-lock.json though... perhaps you did an "npm" command instead of "yarn" or something? FYI- I think I see an unintentional files.txt in your PR too...

As for the test configuration change, yes please! Other than things that are actually platform-specific we want to keep the platforms as in-sync as possible. It's a bit painful but it makes for healthy and consistent codebases. So the effort is appreciated. :-)

- make _areTimestampsInSnapshotsEnabled a regular function instead of a property (underscore in the name doesn't work well with the minifier); - remove type annotations from code snippets in the log warning, and change reference to `app` (presumably a variable defined previously) to `firebase.app()`; - undo most of formatting changes (the exception is breaking lines above the column limit); - restore alphabetical order of imports.

…havior will not be retained)

var-const · 2018-03-28T22:59:27Z

@mikelehen Michael, could you please take another look? The only change since the last time is slight tweaking of warning due to feedback on the doc. The idea was to phrase it clearer that the current behavior is not going to be supported in the future. I'll be happy to change if you'd like.

mikelehen

LGTM with minor nit and unfortunately we need to update a couple more index.d.ts files. :-/

mikelehen · 2018-03-29T16:22:39Z

packages/firestore-types/index.d.ts

@@ -35,6 +35,26 @@ export interface Settings {
  host?: string;
  /** Whether to use SSL when connecting. */
  ssl?: boolean;
+


So... it turns out our types exist in 3 .d.ts files. 😦 @jshcrowthe has plans to fix this, but for now can you update packages/firebase/index.d.ts and packages/firebase/app/index.d.ts too?

Thanks for the tip!

mikelehen · 2018-03-29T16:25:28Z

packages/firestore/src/api/database.ts

+To hide this warning and ensure your app does not break, you need to add the
+following code to your app before calling any other Cloud Firestore methods:
+
+  const firestore = firebase.firestore(firebase.app());


I think firebase.app() is not necessary? Just doing firebase.firestore() should use the default app...

Done.

Just out of curiosity, where is the code handling this?

So it's a bit convoluted. firebase.firestore() is actually a function that FirebaseApp creates when we (Firestore) call firebase.INTERNAL.registerService() to register ourselves. So:

We call firebase.INTERNAL.registerService() here:

firebase-js-sdk/packages/firestore/src/platform/config.ts

Line 58 in b5aa5b8

(firebase as _FirebaseNamespace).INTERNAL.registerService(

It creates this serviceNamespace which is the firebase.firestore() function [see line 369] except it also has properties like firebase.firestore.FieldValue hence it's called a "namespace":

firebase-js-sdk/packages/app/src/firebaseApp.ts

Line 353 in b5aa5b8

const serviceNamespace = (appArg: FirebaseApp = app()) => {

That serviceNamespace function defaults to using the default app (appArg: FirebaseApp = app()).

So app is optional...

Thanks! I tried finding it in the code, realized it's something dynamically created and gave up.

…face is contained

mikelehen

Still LGTM. Tiny optional suggestion for changelog.

mikelehen · 2018-03-30T15:21:55Z

packages/firestore/CHANGELOG.md

@@ -2,6 +2,14 @@
 - [fixed] Fixed a regression in the Firebase JS release 4.11.0 that could
  cause get() requests made while offline to be delayed by up to 10
  seconds (rather than returning from cache immediately).
+- [feature] Added a new `Timestamp` class to represent timestamp fields,
+  currently supporting up to microsecond precision. It can be passed to API
+  methods anywhere a system Date is currently accepted. To make


optional nit: 'JS Date object' might be clearer than 'system Date' (here and below)

jshcrowthe

Non package specific changes LGTM 👍

var-const added 4 commits March 2, 2018 19:27

Make Timestamp a part of the public Firestore API.

fc8d65a

This is a transitional state; eventually, Timestamp will live in a new separate package (common-types/). Also: - move Timestamp.fromISOString method into Serializer, the only place of usage; - rename nanos to nanoseconds.

Accept Timestamps as a parameter where Dates are currently accepted.

9aa7aed

Truncate Timestamps to microseconds immediately upon receiving them as a parameter, so that offline and online behavior is consistent.

[AUTOMATED]: Prettier Code Styling

a839da2

var-const requested review from mikelehen, schmidt-sebastian and wilhuff as code owners March 3, 2018 00:49

google-oss-bot added the needs-triage label Mar 3, 2018

var-const self-assigned this Mar 3, 2018

var-const removed request for wilhuff and schmidt-sebastian March 3, 2018 00:59

var-const commented Mar 3, 2018

View reviewed changes

mikelehen reviewed Mar 3, 2018

View reviewed changes

mikelehen suggested changes Mar 6, 2018

View reviewed changes

var-const requested a review from jshcrowthe as a code owner March 8, 2018 21:17

var-const commented Mar 8, 2018

View reviewed changes

Review feedback:

ff1583d

- add additional comments in Timestamp declarations; - remove unnecessary method declarations from Timestamp; - rename timestampsInSnapshotsEnabled -> timestampsInSnapshots; - use timestampsInSnapshots = true in tests; - small code cleanup.

var-const force-pushed the varconst-timestamp2 branch from 4f58e9d to ff1583d Compare March 8, 2018 23:21

mikelehen reviewed Mar 9, 2018

View reviewed changes

var-const added 3 commits March 9, 2018 17:54

Modify tests to expect the new Timestamp behavior.

27f57da

Also add a test to make sure server timestamps retain the old behavior if the global settings if not set.

[AUTOMATED]: Prettier Code Styling

43ecfd5

Merge branch 'master' into varconst-timestamp2

4b234c0

var-const commented Mar 9, 2018

View reviewed changes

mikelehen approved these changes Mar 9, 2018

View reviewed changes

var-const commented Mar 10, 2018

View reviewed changes

var-const added 4 commits March 9, 2018 19:33

Remove accidentally committed files

48542b6

small fixes

810471a

Tweak wording of the log message (make it clearer that the current be…

d932023

…havior will not be retained)

var-const assigned mikelehen and unassigned var-const Mar 28, 2018

Merge branch 'master' into varconst-timestamp2

8b1bc61

mikelehen approved these changes Mar 29, 2018

View reviewed changes

mikelehen removed their assignment Mar 29, 2018

var-const added 6 commits March 29, 2018 19:07

Add interface changes to other index.d.ts files where Firestore inter…

5272888

…face is contained

[AUTOMATED]: Prettier Code Styling

7137dcb

Remove redundant call from code snippet

614bae3

Add changelog entry

e6bee7d

Remove accidentally committed file

305ca7a

Merge branch 'master' into varconst-timestamp2

bd20865

mikelehen approved these changes Mar 30, 2018

View reviewed changes

jshcrowthe reviewed Mar 30, 2018

View reviewed changes

jshcrowthe approved these changes Mar 30, 2018

View reviewed changes

var-const added 2 commits March 30, 2018 12:55

Tweak changelog

92321ac

Changelog entry: remove hyphen

4d186a4

var-const merged commit 1608edb into master Mar 30, 2018

jshcrowthe deleted the varconst-timestamp2 branch April 16, 2018 21:58

firebase locked and limited conversation to collaborators Oct 22, 2019


		compareTo(other: Timestamp): number;

		isEqual(other: Timestamp): boolean;

Make Timestamp a part of the public Firestore API. #544

Make Timestamp a part of the public Firestore API. #544

Conversation

var-const commented Mar 3, 2018

var-const left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

var-const left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

var-const left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikelehen left a comment

Choose a reason for hiding this comment

var-const left a comment

Choose a reason for hiding this comment

var-const left a comment •

edited

Loading