Skip to content
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

refactor: change timestampsInSnapshots default to true. #520

Merged
merged 2 commits into from
Jan 16, 2019
Merged
Show file tree
Hide file tree
Changes from all 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
61 changes: 33 additions & 28 deletions dev/src/index.ts
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Loading