-
Notifications
You must be signed in to change notification settings - Fork 888
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
add support for set() with SetOptions when using FirestoreDataConverter #3254
Conversation
Binary Size ReportAffected SDKs
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.
Basically LGTM, but we should provide a more helpful error message.
packages/firebase/index.d.ts
Outdated
/** | ||
* Writes to the document referred to by the provided `DocumentReference`. | ||
* If the document does not exist yet, it will be created. If you pass | ||
* `SetOptions`, the provided data can be merged into the existing document. | ||
* | ||
* @param documentRef A reference to the document to be set. | ||
* @param data An object of the fields and values for the document. | ||
* @param options An object to configure the set behavior. | ||
* @return This `Transaction` instance. Used for chaining method calls. | ||
*/ |
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.
This seems to be not needed.
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.
removed.
packages/firestore-types/index.d.ts
Outdated
set<T>( | ||
documentRef: DocumentReference<T>, | ||
data: T | Partial<T>, | ||
options?: SetOptions |
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.
This catch-all override should not be needed in the type file.
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.
removed.
packages/firestore-types/index.d.ts
Outdated
set<T>( | ||
documentRef: DocumentReference<T>, | ||
data: T | Partial<T>, | ||
options?: SetOptions | ||
): WriteBatch; |
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.
This catch-all override should not be needed in the type file.
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.
removed.
packages/firestore-types/index.d.ts
Outdated
set(data: T, options?: SetOptions): Promise<void>; | ||
set(data: Partial<T>, options: SetOptions): Promise<void>; | ||
set(data: T): Promise<void>; | ||
set(data: T | Partial<T>, options?: SetOptions): Promise<void>; |
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.
This catch-all override should not be needed in the type file.
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.
removed.
batch.set(ref, { title: 'olive' }, { merge: true }) | ||
).to.throw( | ||
'Function toFirestore() in WriteBatch.set() called with invalid data. Unsupported field value: undefined (found in field author)' | ||
); |
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.
Ideally, this error message should prompt the user to provide a different converter.
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.
I'll try looking into this some more in a subsequent PR. There doesn't seem to be an easy way to differentiate between the two possible converters once its been compiled down. The only way I can think of is parsing toFirestore.toString()
and checking if the options parameter is included in the function. Do you see an easy implementation here?
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.
The message is generated here:
functionName = 'toFirestore() in ' + functionName; |
Isn't this message always wrong? The output of toFirestore()
is invalid, not the input. Maybe change it to:
functionName = functionName + '() (converted by toFirestore()) ' +;
We need to change the way we pass the parenthesis though.
Since we are still waiting for sign off from @egilmorez, I vote we fix this in this PR.
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.
Done. Doing so cleans up applyFirestoreDataConverter()
as well.
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.
Looks fantastic!
🦋 Changeset is good to goLatest commit: 57ed84b We got this. This PR includes changesets to release 10 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 |
ec8c8e4
to
74aca83
Compare
74aca83
to
89f2c8d
Compare
74e7a77
to
695d0a4
Compare
@@ -314,14 +320,16 @@ export class UserDataReader { | |||
methodName: string, | |||
targetDoc: DocumentKey, | |||
input: unknown, | |||
options: firestore.SetOptions = {} | |||
options: firestore.SetOptions = {}, | |||
hasConverter: boolean |
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.
Nit: Flip argument order to make optional argument go last.
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.
done.
@@ -803,10 +813,16 @@ function createError( | |||
reason: string, | |||
methodName: string, | |||
path?: FieldPath, | |||
targetDoc?: DocumentKey | |||
targetDoc?: DocumentKey, | |||
hasConverter = false |
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.
Would prefer is this was not optional and the third argument. It is easy to miss setting otherwise.
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.
done.
batch.set(ref, { title: 'olive' }, { merge: true }) | ||
).to.throw( | ||
'Function toFirestore() in WriteBatch.set() called with invalid data. Unsupported field value: undefined (found in field author)' | ||
); |
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.
Looks fantastic!
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.
Quick question...
* `SetOptions`, the provided data can be merged into the existing document. | ||
* | ||
* @param documentRef A reference to the document to be set. | ||
* @param data An object of the fields and values for the document. |
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.
Sorry if I'm misreading this, but is SetOptions now required? Do we need a param for it?
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.
We are adding an overload that allows to specify a data: Partial<T>
parameter that must be accompanied by SetOptions
. Using the data: T
overload does not require SetOptions
.
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.
One style nit regarding backticks for code font, otherwise LG, thanks!
packages/firebase/index.d.ts
Outdated
@@ -7859,9 +7859,11 @@ declare namespace firebase.firestore { | |||
/** | |||
* Called by the Firestore SDK to convert a custom model object of type T | |||
* into a plain Javascript object (suitable for writing directly to the | |||
* Firestore database). | |||
* Firestore database). To use set() with `merge` and `mergeFields`, |
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.
I think the literal set()
should have backticks.
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.
done.
packages/firebase/index.d.ts
Outdated
@@ -7859,9 +7859,11 @@ declare namespace firebase.firestore { | |||
/** | |||
* Called by the Firestore SDK to convert a custom model object of type T | |||
* into a plain Javascript object (suitable for writing directly to the | |||
* Firestore database). | |||
* Firestore database). To use set() with `merge` and `mergeFields`, | |||
* toFirestore() must be defined with `Partial<T>`. |
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.
Same here, suggest backticks.
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.
done. thanks for the review!
Porting over from node.