Skip to content

Commit

Permalink
refactor: change timestampsInSnapshots default to true.. (#520)
Browse files Browse the repository at this point in the history 
refactor: change timestampsInSnapshots default to true.
  • Loading branch information
@mikelehen @JustinBeckwith
mikelehen authored and JustinBeckwith committed Jan 16, 2019
1 parent e669f4e commit 5449774
Show file tree
Hide file tree
Showing 8 changed files with 113 additions and 119 deletions.
61 changes: 33 additions & 28 deletions dev/src/index.ts
View file
Expand Up @@ -273,19 +273,20 @@ export class Firestore {
* support {@link https://cloud.google.com/docs/authentication Application
* Default Credentials}. If your credentials are stored in a JSON file, you
* can specify a `keyFilename` instead.
* @param {boolean=} settings.timestampsInSnapshots Enables the use of
* `Timestamp`s for timestamp fields in `DocumentSnapshots`.<br/>
* Currently, Firestore returns 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.
* <br/>Setting `timestampsInSnapshots` to true will cause Firestore to return
* `Timestamp` values instead of `Date` avoiding this kind of problem. To
* make this work you must also change any code that uses `Date` to use
* `Timestamp` instead.
* <br/>NOTE: in the future `timestampsInSnapshots: true` will become the
* default and this option will be removed so you should change your code to
* use `Timestamp` now and opt-in to this new behavior as soon as you can.
* @param {boolean=} settings.timestampsInSnapshots Specifies whether to use
* `Timestamp` objects for timestamp fields in `DocumentSnapshot`s. This is
* enabled by default and should not be disabled.
* <br/>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.
* <br/>So now Firestore returns `Timestamp` values instead of `Date`,
* avoiding this kind of problem.
* <br/>To opt into the old behavior of returning `Date` objects, you can
* temporarily set `timestampsInSnapshots` to false.
* <br/>WARNING: This setting will be removed in a future release. You should
* update your code to expect `Timestamp` objects and stop using the
* `timestampsInSnapshots` setting.
*/
constructor(settings?: Settings) {
this._validator = new Validator({
Expand Down Expand Up @@ -820,32 +821,36 @@ export class Firestore {
private _runRequest<T>(op: (client: GapicClient) => Promise<T>): Promise<T> {
// Initialize the client pool if this is the first request.
if (!this._clientInitialized) {
if (!this._settings.timestampsInSnapshots) {
// Nobody should set timestampsInSnapshots anymore, but the error depends
// on whether they set it to true or false...
if (this._settings.timestampsInSnapshots === true) {
console.error(`
The behavior for Date objects stored in Firestore is going to change
AND YOUR APP MAY BREAK.
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:
The timestampsInSnapshots setting now defaults to true and you no
longer need to explicitly set it. In a future release, the setting
will be removed entirely and so it is recommended that you remove it
from your firestore.settings() call now.`);
} else if (this._settings.timestampsInSnapshots === false) {
console.error(`
The timestampsInSnapshots setting will soon be removed. YOU MUST UPDATE
YOUR CODE.
const firestore = new Firestore();
const settings = {/* your settings... */ timestampsInSnapshots: true};
firestore.settings(settings);
To hide this warning, stop using the timestampsInSnapshots setting in your
firestore.settings({ ... }) call.
With this change, timestamps stored in Cloud Firestore will be read back as
Firebase Timestamp objects instead of as system Date objects. So you will also
need to update code expecting a Date to instead expect a Timestamp. For example:
Once you remove the setting, Timestamps stored in Cloud Firestore will be
read back as Firebase Timestamp objects instead of as system Date objects.
So you will also need to update code expecting a Date to instead expect a
Timestamp. For example:
// Old:
const date = snapshot.get('created_at');
// New:
const timestamp = snapshot.get('created_at');
const date = timestamp.toDate();
Please audit all existing usages of Date when you enable the new behavior. In a
future release, the behavior will change to the new behavior, so if you do not
follow these steps, YOUR APP MAY BREAK.`);
Please audit all existing usages of Date when you enable the new
behavior.`);
}

this._clientInitialized = this._initClientPool().then(clientPool => {
this._clientPool = clientPool;
});
Expand Down
6 changes: 5 additions & 1 deletion dev/src/serializer.ts
View file
Expand Up @@ -49,7 +49,11 @@ export class Serializer {
// its `.doc()` method. This avoid a circular reference, which breaks
// JSON.stringify().
this.createReference = path => firestore.doc(path);
this.timestampsInSnapshots = !!firestore._settings.timestampsInSnapshots;
if (firestore._settings.timestampsInSnapshots === undefined) {
this.timestampsInSnapshots = true;
} else {
this.timestampsInSnapshots = firestore._settings.timestampsInSnapshots;
}
}

/**
Expand Down
25 changes: 13 additions & 12 deletions dev/src/types.ts
View file
Expand Up @@ -67,22 +67,23 @@ export interface Settings {
keyFilename?: string;

/**
* Enables the use of `Timestamp`s for timestamp fields in
* `DocumentSnapshot`s.
* Specifies whether to use `Timestamp` objects for timestamp fields in
* `DocumentSnapshot`s. This is enabled by default and should not be disabled.
*
* Currently, Firestore returns timestamp fields as `Date` but `Date` only
* 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.
* unexpected behavior when using a timestamp from a snapshot as a part of a
* subsequent query.
*
* Setting `timestampsInSnapshots` to true will cause Firestore to return
* `Timestamp` values instead of `Date` avoiding this kind of problem. To
* make this work you must also change any code that uses `Date` to use
* `Timestamp` instead.
* So now Firestore returns `Timestamp` values instead of `Date`, avoiding
* this kind of problem.
*
* NOTE: in the future `timestampsInSnapshots: true` will become the
* default and this option will be removed so you should change your code to
* use `Timestamp` now and opt-in to this new behavior as soon as you can.
* To opt into the old behavior of returning `Date` objects, you can
* temporarily set `timestampsInSnapshots` to false.
*
* @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;

Expand Down
14 changes: 7 additions & 7 deletions dev/system-test/firestore.ts
View file
Expand Up @@ -51,7 +51,7 @@ describe('Firestore class', () => {
let randomCol;

beforeEach(() => {
firestore = new Firestore({timestampsInSnapshots: true});
firestore = new Firestore({});
randomCol = getTestRoot(firestore);
});

Expand Down Expand Up @@ -119,7 +119,7 @@ describe('CollectionReference class', () => {
let randomCol;

beforeEach(() => {
firestore = new Firestore({timestampsInSnapshots: true});
firestore = new Firestore({});
randomCol = getTestRoot(firestore);
});

Expand Down Expand Up @@ -184,7 +184,7 @@ describe('DocumentReference class', () => {
let randomCol;

beforeEach(() => {
firestore = new Firestore({timestampsInSnapshots: true});
firestore = new Firestore({});
randomCol = getTestRoot(firestore);
});

Expand Down Expand Up @@ -878,7 +878,7 @@ describe('Query class', () => {
};

beforeEach(() => {
firestore = new Firestore({timestampsInSnapshots: true});
firestore = new Firestore({});
randomCol = getTestRoot(firestore);
});

Expand Down Expand Up @@ -1373,7 +1373,7 @@ describe('Transaction class', () => {
let randomCol;

beforeEach(() => {
firestore = new Firestore({timestampsInSnapshots: true});
firestore = new Firestore({});
randomCol = getTestRoot(firestore);
});

Expand Down Expand Up @@ -1546,7 +1546,7 @@ describe('WriteBatch class', () => {
let randomCol;

beforeEach(() => {
firestore = new Firestore({timestampsInSnapshots: true});
firestore = new Firestore({});
randomCol = getTestRoot(firestore);
});

Expand Down Expand Up @@ -1640,7 +1640,7 @@ describe('QuerySnapshot class', () => {
let querySnapshot;

beforeEach(() => {
firestore = new Firestore({timestampsInSnapshots: true});
firestore = new Firestore({});

const randomCol = getTestRoot(firestore);
const ref1 = randomCol.doc('doc1');
Expand Down
10 changes: 2 additions & 8 deletions dev/test/index.ts
View file
Expand Up @@ -32,8 +32,7 @@ const DATABASE_ROOT = `projects/${PROJECT_ID}/databases/(default)`;
const DEFAULT_SETTINGS = {
projectId: PROJECT_ID,
sslCreds: grpc.credentials.createInsecure(),
keyFilename: __dirname + '/fake-certificate.json',
timestampsInSnapshots: true
keyFilename: __dirname + '/fake-certificate.json'
};

// Change the argument to 'console.log' to enable debug output.
Expand Down Expand Up @@ -264,7 +263,7 @@ describe('instantiation', () => {

it('can only call settings() once', () => {
const firestore = new Firestore.Firestore(DEFAULT_SETTINGS);
firestore.settings({timestampsInSnapshots: true});
firestore.settings({});

expect(() => firestore.settings({}))
.to.throw(
Expand Down Expand Up @@ -327,7 +326,6 @@ describe('instantiation', () => {
it('detects project id', () => {
const firestore = new Firestore.Firestore({
sslCreds: grpc.credentials.createInsecure(),
timestampsInSnapshots: true,
keyFilename: __dirname + '/fake-certificate.json',
});

Expand All @@ -346,7 +344,6 @@ describe('instantiation', () => {
it('uses project id from gapic client', () => {
const firestore = new Firestore.Firestore({
sslCreds: grpc.credentials.createInsecure(),
timestampsInSnapshots: true,
keyFilename: './test/fake-certificate.json',
});

Expand All @@ -363,7 +360,6 @@ describe('instantiation', () => {
it('uses project ID from settings()', () => {
const firestore = new Firestore.Firestore({
sslCreds: grpc.credentials.createInsecure(),
timestampsInSnapshots: true,
keyFilename: './test/fake-certificate.json',
});

Expand All @@ -376,7 +372,6 @@ describe('instantiation', () => {
it('handles error from project ID detection', () => {
const firestore = new Firestore.Firestore({
sslCreds: grpc.credentials.createInsecure(),
timestampsInSnapshots: true,
keyFilename: './test/fake-certificate.json',
});

Expand Down Expand Up @@ -478,7 +473,6 @@ describe('snapshot_() method', () => {
firestore = new Firestore.Firestore({
projectId: PROJECT_ID,
sslCreds: grpc.credentials.createInsecure(),
timestampsInSnapshots: true,
keyFilename: './test/fake-certificate.json',
});
});
Expand Down
78 changes: 34 additions & 44 deletions dev/test/timestamp.ts
View file
Expand Up @@ -49,16 +49,14 @@ const DOCUMENT_WITH_EMPTY_TIMESTAMP = document('documentId', 'moonLanding', {
});

describe('timestamps', () => {
it('returned when enabled', () => {
return createInstance(
{timestampsInSnapshots: true}, DOCUMENT_WITH_TIMESTAMP)
.then(firestore => {
const expected = new Firestore.Timestamp(-14182920, 123000123);
return firestore.doc('collectionId/documentId').get().then(res => {
expect(res.data()!['moonLanding'].isEqual(expected)).to.be.true;
expect(res.get('moonLanding')!.isEqual(expected)).to.be.true;
});
});
it('returned by default', () => {
return createInstance({}, DOCUMENT_WITH_TIMESTAMP).then(firestore => {
const expected = new Firestore.Timestamp(-14182920, 123000123);
return firestore.doc('collectionId/documentId').get().then(res => {
expect(res.data()!['moonLanding'].isEqual(expected)).to.be.true;
expect(res.get('moonLanding')!.isEqual(expected)).to.be.true;
});
});
});

it('converted to dates when disabled', () => {
Expand All @@ -79,50 +77,42 @@ describe('timestamps', () => {
});

it('retain seconds and nanoseconds', () => {
return createInstance(
{timestampsInSnapshots: true}, DOCUMENT_WITH_TIMESTAMP)
.then(firestore => {
return firestore.doc('collectionId/documentId').get().then(res => {
const timestamp = res.get('moonLanding');
expect(timestamp.seconds).to.equal(-14182920);
expect(timestamp.nanoseconds).to.equal(123000123);
});
});
return createInstance({}, DOCUMENT_WITH_TIMESTAMP).then(firestore => {
return firestore.doc('collectionId/documentId').get().then(res => {
const timestamp = res.get('moonLanding');
expect(timestamp.seconds).to.equal(-14182920);
expect(timestamp.nanoseconds).to.equal(123000123);
});
});
});

it('convert to date', () => {
return createInstance(
{timestampsInSnapshots: true}, DOCUMENT_WITH_TIMESTAMP)
.then(firestore => {
return firestore.doc('collectionId/documentId').get().then(res => {
const timestamp = res.get('moonLanding');
expect(new Date(-14182920 * 1000 + 123).getTime())
.to.equal(timestamp.toDate().getTime());
});
});
return createInstance({}, DOCUMENT_WITH_TIMESTAMP).then(firestore => {
return firestore.doc('collectionId/documentId').get().then(res => {
const timestamp = res.get('moonLanding');
expect(new Date(-14182920 * 1000 + 123).getTime())
.to.equal(timestamp.toDate().getTime());
});
});
});

it('convert to millis', () => {
return createInstance(
{timestampsInSnapshots: true}, DOCUMENT_WITH_TIMESTAMP)
.then(firestore => {
return firestore.doc('collectionId/documentId').get().then(res => {
const timestamp = res.get('moonLanding');
expect(-14182920 * 1000 + 123).to.equal(timestamp.toMillis());
});
});
return createInstance({}, DOCUMENT_WITH_TIMESTAMP).then(firestore => {
return firestore.doc('collectionId/documentId').get().then(res => {
const timestamp = res.get('moonLanding');
expect(-14182920 * 1000 + 123).to.equal(timestamp.toMillis());
});
});
});

it('support missing values', () => {
return createInstance(
{timestampsInSnapshots: true}, DOCUMENT_WITH_EMPTY_TIMESTAMP)
.then(firestore => {
const expected = new Firestore.Timestamp(0, 0);
return createInstance({}, DOCUMENT_WITH_EMPTY_TIMESTAMP).then(firestore => {
const expected = new Firestore.Timestamp(0, 0);

return firestore.doc('collectionId/documentId').get().then(res => {
expect(res.get('moonLanding').isEqual(expected)).to.be.true;
});
});
return firestore.doc('collectionId/documentId').get().then(res => {
expect(res.get('moonLanding').isEqual(expected)).to.be.true;
});
});
});

it('constructed using helper', () => {
Expand Down
1 change: 0 additions & 1 deletion dev/test/util/helpers.ts
View file
Expand Up @@ -87,7 +87,6 @@ export function createInstance(
{
projectId: PROJECT_ID,
sslCreds: SSL_CREDENTIALS,
timestampsInSnapshots: true,
keyFilename: __dirname + '/../fake-certificate.json',
},
firestoreSettings);
Expand Down

0 comments on commit 5449774

Please sign in to comment.