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.

Sign up for GitHub

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

Jump to bottom

feat!: remove deprecated timestampInSnapshots setting #808

Merged
merged 2 commits into from
Dec 9, 2019
Merged

feat!: remove deprecated timestampInSnapshots setting #808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
959c41f
feat!: remove deprecated timestampInSnapshots setting
schmidt-sebastian Dec 6, 2019
bc51b45
Merge branch 'master' into mrschmidt/deprecate
bcoe Dec 6, 2019
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
64 changes: 1 addition & 63 deletions dev/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ export class Firestore {
* The configuration options for the GAPIC client.
* @private
*/
_settings: Settings = {};
private _settings: Settings = {};

/**
* Whether the initialization settings can still be changed by invoking
Expand Down Expand Up @@ -317,20 +317,6 @@ export class Firestore {
* can specify a `keyFilename` instead.
* @param {string=} settings.host The host to connect to.
* @param {boolean=} settings.ssl Whether to use SSL when connecting.
* @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) {
const libraryHeader = {
Expand Down Expand Up @@ -420,12 +406,6 @@ export class Firestore {
settings(settings: Settings): void {
validateObject('settings', settings);
validateString('settings.projectId', settings.projectId, {optional: true});
validateBoolean(
'settings.timestampsInSnapshots',
// tslint:disable-next-line deprecation
settings.timestampsInSnapshots,
{optional: true}
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I reading the code correctly that we don't have any particular validation to prevent "extra" settings? (i.e. if a user typos 'projectId' as 'projectID' we'll just silently ignore it rather than let them know that they messed up?)

It would be nice if we had validation like the web SDK (https://github.com/firebase/firebase-js-sdk/blob/57a0f2bd78afba64b292125522f1071b386a920d/packages/firestore/src/api/database.ts#L181) and then now that we remove timestampsInSnapshots, people will get a very clear error if they're using it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we don't have this validation. The Server SDK supports a ton of options that are consumed by either our code, Google Gax or GRPC. I would be surprised if anyone knew the complete set of settings we support, hence I don't think we can validate them here.


if (this._settingsFrozen) {
throw new Error(
Expand All @@ -441,13 +421,6 @@ export class Firestore {
}

private validateAndApplySettings(settings: Settings): void {
validateBoolean(
'settings.timestampsInSnapshots',
// tslint:disable-next-line deprecation
settings.timestampsInSnapshots,
{optional: true}
);

if (settings.projectId !== undefined) {
validateString('settings.projectId', settings.projectId);
this._projectId = settings.projectId;
Expand Down Expand Up @@ -1043,41 +1016,6 @@ export class Firestore {
* @return A Promise that resolves when the client is initialized.
*/
async initializeIfNeeded(requestTag: string): Promise<void> {
if (!this._settingsFrozen) {
// Nobody should set timestampsInSnapshots anymore, but the error depends
// on whether they set it to true or false...
// tslint:disable-next-line deprecation
if (this._settings.timestampsInSnapshots === true) {
console.error(`
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.`);
// tslint:disable-next-line deprecation
} else if (this._settings.timestampsInSnapshots === false) {
console.error(`
The timestampsInSnapshots setting will soon be removed. YOU MUST UPDATE
YOUR CODE.

To hide this warning, stop using the timestampsInSnapshots setting in your
firestore.settings({ ... }) call.

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.`);
}
}

this._settingsFrozen = true;

if (this._projectId === undefined) {
Expand Down
10 changes: 1 addition & 9 deletions dev/src/serializer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,21 +52,13 @@ export interface Serializable {
* @private
*/
export class Serializer {
private timestampsInSnapshots: boolean;
private createReference: (path: string) => DocumentReference;

constructor(firestore: Firestore) {
// Instead of storing the `firestore` object, we store just a reference to
// its `.doc()` method. This avoid a circular reference, which breaks
// JSON.stringify().
this.createReference = path => firestore.doc(path);
// tslint:disable-next-line deprecation
if (firestore._settings.timestampsInSnapshots === undefined) {
this.timestampsInSnapshots = true;
} else {
// tslint:disable-next-line deprecation
this.timestampsInSnapshots = firestore._settings.timestampsInSnapshots;
}
}

/**
Expand Down Expand Up @@ -218,7 +210,7 @@ export class Serializer {
}
case 'timestampValue': {
const timestamp = Timestamp.fromProto(proto.timestampValue!);
return this.timestampsInSnapshots ? timestamp : timestamp.toDate();
return timestamp;
}
case 'referenceValue': {
const resourcePath = QualifiedResourcePath.fromSlashSeparatedString(
Expand Down
21 changes: 0 additions & 21 deletions dev/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,27 +69,6 @@ export interface Settings {
*/
credentials?: {client_email?: string; private_key?: string};

/**
* 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.
*
* So now Firestore returns `Timestamp` values instead of `Date`, avoiding
* this kind of problem.
*
* 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;

/** Whether to use SSL when connecting. */
ssl?: boolean;

Expand Down
23 changes: 4 additions & 19 deletions dev/test/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,8 +301,10 @@ describe('instantiation', () => {
const firestore = new Firestore.Firestore(DEFAULT_SETTINGS);
firestore.settings({foo: 'bar'});

expect(firestore._settings.projectId).to.equal(PROJECT_ID);
expect(firestore._settings.foo).to.equal('bar');
/* tslint:disable:no-any */
expect((firestore as any)._settings.projectId).to.equal(PROJECT_ID);
expect((firestore as any)._settings.foo).to.equal('bar');
/* tslint:enable:no-any */
});

it('can only call settings() once', () => {
Expand Down Expand Up @@ -340,23 +342,6 @@ describe('instantiation', () => {
);
});

it('validates timestampsInSnapshots is boolean', () => {
expect(() => {
const settings = {...DEFAULT_SETTINGS, timestampsInSnapshots: 1337};
new Firestore.Firestore(settings as InvalidApiUsage);
}).to.throw(
'Value for argument "settings.timestampsInSnapshots" is not a valid boolean.'
);

expect(() => {
new Firestore.Firestore(DEFAULT_SETTINGS).settings({
timestampsInSnapshots: 1337 as InvalidApiUsage,
});
}).to.throw(
'Value for argument "settings.timestampsInSnapshots" is not a valid boolean.'
);
});

it('validates ssl is a boolean', () => {
const invalidValues = ['true', 1337];

Expand Down
21 changes: 0 additions & 21 deletions dev/test/timestamp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,27 +67,6 @@ describe('timestamps', () => {
});
});

it('converted to dates when disabled', () => {
const oldErrorLog = console.error;
// Prevent error message that prompts to enable `timestampsInSnapshots`
// behavior.
console.error = () => {};

return createInstance(
{timestampsInSnapshots: false},
DOCUMENT_WITH_TIMESTAMP
).then(firestore => {
return firestore
.doc('collectionId/documentId')
.get()
.then(res => {
expect(res.data()!['moonLanding']).to.be.instanceOf(Date);
expect(res.get('moonLanding')).to.be.instanceOf(Date);
console.error = oldErrorLog;
});
});
});

it('retain seconds and nanoseconds', () => {
return createInstance({}, DOCUMENT_WITH_TIMESTAMP).then(firestore => {
return firestore
Expand Down
2 changes: 0 additions & 2 deletions dev/test/typescript.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ xdescribe('firestore.d.ts', () => {
const firestore: Firestore = new Firestore({
keyFilename: 'foo',
projectId: 'foo',
timestampsInSnapshots: true,
host: 'localhost',
ssl: false,
otherOption: 'foo',
Expand All @@ -59,7 +58,6 @@ xdescribe('firestore.d.ts', () => {
firestore.settings({
keyFilename: 'foo',
projectId: 'foo',
timestampsInSnapshots: true,
otherOption: 'foo',
});
const collRef: CollectionReference = firestore.collection('coll');
Expand Down
21 changes: 0 additions & 21 deletions types/firestore.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,27 +77,6 @@ declare namespace FirebaseFirestore {
*/
credentials?: {client_email?:string, private_key?:string};

/**
* 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.
*
* So now Firestore returns `Timestamp` values instead of `Date`, avoiding
* this kind of problem.
*
* 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;

/** Whether to use SSL when connecting. */
ssl?: boolean;

Expand Down