-
Notifications
You must be signed in to change notification settings - Fork 893
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
Conversation
🦋 Changeset detectedLatest commit: 307da2a The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
Size Report 1Affected ProductsNo changes between base commit (b00b54b) and merge commit (9846647).Test Logs |
Size Analysis Report 1Affected ProductsNo changes between base commit (b00b54b) and merge commit (9846647).Test Logs |
There was a problem hiding this 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.
There was a problem hiding this 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.
@markarndt PTAL for docs review |
This was included in the v10.0.0 release: https://firebase.google.com/support/release-notes/js#version_1000_-_july_6_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 withupdateDoc()
,Transaction.update()
, andWriteBatch.update()
where erroneous TypeScript compilation errors would occur if the shape of the data written to Firestore was different from the shape of theT
type parameter of theFirestoreDataConverter
.Background Information
The
DocumentReference
,CollectionReference
, andQuery
classes in Firestore all provide awithConverter()
method that associates aFirestoreDataConverter
with the object. Once registered, any operations that read data from or write data to Firestore via the object will go through theFirestoreDataConverter
. 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 theFirestoreDataConverter
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 likesetDoc()
,addDoc()
,Transaction.set()
andWriteBatch.set()
and the object would be converted to the database representation via theFirestoreDataConverter
before writing it to Firestore. Similarly, the snapshot returned from functions likegetDoc()
andTransaction.get()
would use theFirestoreDataConverter
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 typeT
. This worked as long as the properties ofT
were exactly those written to Firestore; however, sinceupdateDoc()
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 toany
, 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 andupdateDoc()
,Transaction.update()
andWriteBatch.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 yourFirestoreDataConverter
.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 tofunction 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.