Skip to content

Remove error messages for deprecated APIs #2857

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

Merged
merged 12 commits into from
Apr 6, 2020
22 changes: 17 additions & 5 deletions packages/firebase/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7682,11 +7682,23 @@ declare namespace firebase.firestore {
* 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.
* 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
Expand Down
82 changes: 9 additions & 73 deletions packages/firestore/src/api/database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,32 +194,15 @@ class FirestoreSettings {
// Nobody should set timestampsInSnapshots anymore, but the error depends on
// whether they set it to true or false...
if (settings.timestampsInSnapshots === true) {
logError(`
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.`);
logError(
"The setting 'timestampsInSnapshots: true' is no longer required " +
'and should be removed.'
);
} else if (settings.timestampsInSnapshots === false) {
logError(`
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.`);
logError(
"Support for 'timestampsInSnapshots: false' will be removed soon. " +
'You must update your code to handle Timestamp objects.'
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of putting the helpful text about how to handle this in a file like packages/firestore/deprecation.md and then linked to it? Then this could be

"Support for 'timestampsInSnapshots: false' will be removed. " +
  'See https://github.com/firebase/firebase-js-sdk/packages/firestore/deprecation.md#timestamps'

It's almost as short but remains just as helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I expanded on the comment in our API docs and added a GIST with the comment (since we can't permalink to the API doc).

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW I went back to the original error message (but left the updates to the API documentation). This error message is so old that the number of people that see this probably does not warrant any extra steps.

);
}
this.timestampsInSnapshots =
settings.timestampsInSnapshots ?? DEFAULT_TIMESTAMPS_IN_SNAPSHOTS;
Expand Down Expand Up @@ -338,14 +321,6 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
validateExactNumberOfArgs('Firestore.settings', arguments, 1);
validateArgType('Firestore.settings', 'object', 1, settingsLiteral);

if (contains(settingsLiteral, 'persistence')) {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
'"persistence" is now specified with a separate call to ' +
'firestore.enablePersistence().'
);
}

const newSettings = new FirestoreSettings(settingsLiteral);
if (this._firestoreClient && !this._settings.isEqual(newSettings)) {
throw new FirestoreError(
Expand Down Expand Up @@ -387,10 +362,7 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
if (settings) {
if (settings.experimentalTabSynchronization !== undefined) {
logError(
"The 'experimentalTabSynchronization' setting has been renamed to " +
"'synchronizeTabs'. In a future release, the setting will be removed " +
'and it is recommended that you update your ' +
"firestore.enablePersistence() call to use 'synchronizeTabs'."
"The 'experimentalTabSynchronization' setting will be removed. Use 'synchronizeTabs' instead."
);
}
synchronizeTabs =
Expand Down Expand Up @@ -2305,42 +2277,6 @@ export class QuerySnapshot<T = firestore.DocumentData>
}
}

// TODO(2018/11/01): As of 2018/04/17 we're changing docChanges from an array
// into a method. Because this is a runtime breaking change and somewhat subtle
// (both Array and Function have a .length, etc.), we'll replace commonly-used
// properties (including Symbol.iterator) to throw a custom error message. In
// ~6 months we can delete the custom error as most folks will have hopefully
// migrated.
function throwDocChangesMethodError(): never {
throw new FirestoreError(
Code.INVALID_ARGUMENT,
'QuerySnapshot.docChanges has been changed from a property into a ' +
'method, so usages like "querySnapshot.docChanges" should become ' +
'"querySnapshot.docChanges()"'
);
}

const docChangesPropertiesToOverride = [
'length',
'forEach',
'map',
...(typeof Symbol !== 'undefined' ? [Symbol.iterator] : [])
];
docChangesPropertiesToOverride.forEach(property => {
/**
* We are (re-)defining properties on QuerySnapshot.prototype.docChanges which
* is a Function. This could fail, in particular in the case of 'length' which
* already exists on Function.prototype and on IE11 is improperly defined with
* `{ configurable: false }`. So we wrap this in a try/catch to ensure that we
* still have a functional SDK.
*/
try {
Object.defineProperty(QuerySnapshot.prototype.docChanges, property, {
get: () => throwDocChangesMethodError()
});
} catch (err) {} // Ignore this failure intentionally
});

export class CollectionReference<T = firestore.DocumentData> extends Query<T>
implements firestore.CollectionReference<T> {
constructor(
Expand Down
18 changes: 1 addition & 17 deletions packages/firestore/test/integration/api/query.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2017 Google Inc.
* Copyright 2017 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -784,22 +784,6 @@ apiDescribe('Queries', (persistence: boolean) => {
});
});

it('throws custom error when using docChanges as property', () => {
return withTestCollection(persistence, {}, async coll => {
const snapshot = await coll.get();
const expectedError =
'QuerySnapshot.docChanges has been changed from a property into a method';
// We are testing invalid API usage.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const docChange = snapshot.docChanges as any;
expect(() => docChange.length).to.throw(expectedError);
expect(() => {
for (const _ of docChange) {
}
}).to.throw(expectedError);
});
});

it('can query collection groups', async () => {
await withTestDb(persistence, async db => {
// Use .doc() to get a random collection group name to use but ensure it starts with 'b' for
Expand Down