Firestore: Fix the updateDoc() incorrect parameter type issue #7310
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 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 |
dconeybe
requested review from
a team,
egilmorez,
markarndt,
kevinthecheung,
dwyfrequency and
hsubox76
as code owners
There was a problem hiding this comment.
@dconeybe I've been reading through. Here's a few thoughts for your consideration. I've not seen anything of major concern.
MarkDuckworth
approved these changes
Jun 29, 2023
|
@markarndt PTAL for docs review |
markarndt
approved these changes
Jun 30, 2023
hsubox76
approved these changes
Jun 30, 2023
Merged
|
This was included in the v10.0.0 release: https://firebase.google.com/support/release-notes/js#version_1000_-_july_6_2023 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a 2nd type parameter to
FirestoreDataConverterto 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 theTtype parameter of theFirestoreDataConverter.Background Information
The
DocumentReference,CollectionReference, andQueryclasses in Firestore all provide awithConverter()method that associates aFirestoreDataConverterwith 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 theFirestoreDataConverterconverts objects in both directions.Previously, the
FirestoreDataConverteronly 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 theFirestoreDataConverterbefore writing it to Firestore. Similarly, the snapshot returned from functions likegetDoc()andTransaction.get()would use theFirestoreDataConverterto 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 ofTwere 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
FirestoreDataConverterto 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.