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

Firestore: Fix the updateDoc() incorrect parameter type issue #7310

Merged
merged 14 commits into from
Jun 30, 2023

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented May 19, 2023

Add a 2nd type parameter to FirestoreDataConverter to specify the shape of the data stored in Firestore so that it can be different than the high-level object that is used to represent the data in the rest of the application. This fixes a long-standing typing issue with updateDoc(), Transaction.update(), and WriteBatch.update() where erroneous TypeScript compilation errors would occur if the shape of the data written to Firestore was different from the shape of the T type parameter of the FirestoreDataConverter.

Background Information

The DocumentReference, CollectionReference, and Query classes in Firestore all provide a withConverter() method that associates a FirestoreDataConverter with the object. Once registered, any operations that read data from or write data to Firestore via the object will go through the FirestoreDataConverter. The primary use case of the converter is to transparently convert between a high-level application object, that is convenient for use in the application, and a low-level plain-old-data JavaScript object, that is actually stored in Firestore. These two objects do not necessarily have the same properties (although they can). So the FirestoreDataConverter converts objects in both directions.

Previously, the FirestoreDataConverter only had a single type parameter, T, which indicated the "high-level application object". This object type would then be specified to functions like setDoc(), addDoc(), Transaction.set() and WriteBatch.set() and the object would be converted to the database representation via the FirestoreDataConverter before writing it to Firestore. Similarly, the snapshot returned from functions like getDoc() and Transaction.get() would use the FirestoreDataConverter to convert from the raw data read from Firestore to the high-level object.

The problem was that the updateDoc() function was parameterized to also accept an object of type T. This worked as long as the properties of T were exactly those written to Firestore; however, since updateDoc() works on the raw data written to Firestore, if those properties were different then an incorrect TypeScript compiler error would occur. Customers often worked around this compilation error by casting the data argument to the "update" methods to any, basically turning off the type checks.

The Fix

The fix in this PR is to separate the specification of the high-level application object type and the low-level plain-old-data type that is actually stored in Firestore. It adds a 2nd type parameter to FirestoreDataConverter to specify this type and updateDoc(), Transaction.update() and WriteBatch.update() have their type parameter changed to accept this low-level type instead of the high-level type. TypeScript now produces accurate compilation errors, and also highlights that the "update" family of methods work on the low-level type rather than the high-level type.

Updating Your Code

If your application is written in pure JavaScript (rather than TypeScript) then you will not need to change anything. The changes in this PR are purely in the TypeScript typing system. However, if your application uses TypeScript, you may experience some new TypeScript compilation errors. You may need to update the argument types and return types of your implementations of FirestoreDataConverter. You may also want to create an interface for the low-level database type and specify it as the 2nd type parameter for your FirestoreDataConverter.

Finally, if you have functions that forward Firestore types like DocumentReference you may need to add a 2nd type parameter to forward it as well. For example, suppose you have a function function foo<T>(documentRef: DocumentReference<T>) then you may want to change it to function foo<AppModelType, DbModelType extends DocumentData>(documentRef: DocumentReference<AppModelType, DbModelType>) so that the arguments are perfectly forwarded.

Googlers see b/256607394 and go/firestore-updatedata-type-fix-api-proposal for details.

@dconeybe dconeybe self-assigned this May 19, 2023
@changeset-bot
Copy link

changeset-bot bot commented May 19, 2023

🦋 Changeset detected

Latest commit: 307da2a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@firebase/firestore Major
firebase Major
@firebase/firestore-compat Patch
@firebase/rules-unit-testing Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@google-oss-bot
Copy link
Contributor

google-oss-bot commented May 19, 2023

@google-oss-bot
Copy link
Contributor

google-oss-bot commented May 19, 2023

@hsubox76 hsubox76 added this to the v10 milestone Jun 26, 2023
Copy link
Contributor

@MarkDuckworth MarkDuckworth left a comment

Choose a reason for hiding this comment

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

@dconeybe I've been reading through. Here's a few thoughts for your consideration. I've not seen anything of major concern.

Copy link
Contributor

@MarkDuckworth MarkDuckworth left a comment

Choose a reason for hiding this comment

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

LGTM. Nothing in my review should prevent merging, but please consider comments for changes now or in the future.

@dconeybe
Copy link
Contributor Author

@markarndt PTAL for docs review

@dconeybe
Copy link
Contributor Author

dconeybe commented Jul 7, 2023

This was included in the v10.0.0 release: https://firebase.google.com/support/release-notes/js#version_1000_-_july_6_2023

@firebase firebase locked and limited conversation to collaborators Jul 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants