add support for set() with SetOptions when using FirestoreDataConverter #3254

thebrianchen · 2020-06-22T23:32:11Z

Porting over from node.

google-oss-bot · 2020-06-22T23:40:29Z

Binary Size Report

Affected SDKs

@firebase/firestore

Type Base (64d9ab6) Head (d0cfccc) Diff

browser 248 kB 248 kB +186 B (+0.1%)

esm2017 194 kB 194 kB +217 B (+0.1%)

main 493 kB 494 kB +955 B (+0.2%)

module 246 kB 246 kB +186 B (+0.1%)
@firebase/firestore/exp

Type Base (64d9ab6) Head (d0cfccc) Diff

main 37.4 kB 37.5 kB +130 B (+0.3%)
@firebase/firestore/lite

Type Base (64d9ab6) Head (d0cfccc) Diff

main 140 kB 140 kB +293 B (+0.2%)
@firebase/firestore/memory

Type Base (64d9ab6) Head (d0cfccc) Diff

browser 186 kB 186 kB +148 B (+0.1%)

esm2017 145 kB 145 kB +175 B (+0.1%)

main 363 kB 363 kB +631 B (+0.2%)

module 184 kB 184 kB +148 B (+0.1%)

`firebase`

Type	Base (`64d9ab6`)	Head (`d0cfccc`)	Diff
firebase-firestore.js	287 kB	287 kB	+186 B (+0.1%)
firebase-firestore.memory.js	226 kB	226 kB	+148 B (+0.1%)
firebase.js	820 kB	820 kB	+186 B (+0.0%)

Test Logs

Base (64d9ab6b): https://github.com/firebase/firebase-js-sdk/actions/runs/148109477
Head (d0cfccce): https://github.com/firebase/firebase-js-sdk/actions/runs/149049539

schmidt-sebastian

Basically LGTM, but we should provide a more helpful error message.

schmidt-sebastian · 2020-06-23T16:45:52Z

packages/firebase/index.d.ts

+    /**
+     * 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.
+     */


This seems to be not needed.

schmidt-sebastian · 2020-06-23T16:47:23Z

packages/firestore-types/index.d.ts

+  set<T>(
+    documentRef: DocumentReference<T>,
+    data: T | Partial<T>,
    options?: SetOptions


This catch-all override should not be needed in the type file.

schmidt-sebastian · 2020-06-23T16:47:28Z

packages/firestore-types/index.d.ts

+  set<T>(
+    documentRef: DocumentReference<T>,
+    data: T | Partial<T>,
    options?: SetOptions
  ): WriteBatch;


This catch-all override should not be needed in the type file.

schmidt-sebastian · 2020-06-23T16:47:36Z

packages/firestore-types/index.d.ts

-  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>;


This catch-all override should not be needed in the type file.

schmidt-sebastian · 2020-06-23T16:49:38Z

packages/firestore/test/integration/api/database.test.ts

+          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)'
+        );


Ideally, this error message should prompt the user to provide a different converter.

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?

The message is generated here:

firebase-js-sdk/packages/firestore/src/api/database.ts

Line 2647 in f4451f0

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.

Done. Doing so cleans up applyFirestoreDataConverter() as well.

Looks fantastic!

changeset-bot · 2020-06-25T17:16:10Z

🦋 Changeset is good to go

Latest commit: 57ed84b

We got this.

This PR includes changesets to release 10 packages

Name	Type
firebase	Minor
@firebase/firestore	Minor
@firebase/firestore-types	Minor
rxfire	Major
@firebase/testing	Patch
firebase-browserify-test	Patch
firebase-package-typings-test	Patch
firebase-messaging-selenium-test	Patch
firebase-typescript-test	Patch
firebase-webpack-test	Patch

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

.changeset/rare-rabbits-grab.md

schmidt-sebastian · 2020-06-26T05:11:39Z

packages/firestore/src/api/user_data_reader.ts

@@ -314,14 +320,16 @@ export class UserDataReader {
    methodName: string,
    targetDoc: DocumentKey,
    input: unknown,
-    options: firestore.SetOptions = {}
+    options: firestore.SetOptions = {},
+    hasConverter: boolean


Nit: Flip argument order to make optional argument go last.

schmidt-sebastian · 2020-06-26T05:12:59Z

packages/firestore/src/api/user_data_reader.ts

@@ -803,10 +813,16 @@ function createError(
  reason: string,
  methodName: string,
  path?: FieldPath,
-  targetDoc?: DocumentKey
+  targetDoc?: DocumentKey,
+  hasConverter = false


Would prefer is this was not optional and the third argument. It is easy to miss setting otherwise.

schmidt-sebastian · 2020-06-26T05:13:31Z

packages/firestore/test/integration/api/database.test.ts

+          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)'
+        );


Looks fantastic!

markarndt

Quick question...

markarndt · 2020-06-26T18:00:07Z

packages/firebase/index.d.ts

+     * `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.


Sorry if I'm misreading this, but is SetOptions now required? Do we need a param for it?

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.

egilmorez

One style nit regarding backticks for code font, otherwise LG, thanks!

egilmorez · 2020-06-26T19:02:42Z

packages/firebase/index.d.ts

@@ -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`,


I think the literal set() should have backticks.

egilmorez · 2020-06-26T19:03:05Z

packages/firebase/index.d.ts

@@ -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>`.


Same here, suggest backticks.

done. thanks for the review!

add set override

f6c62da

thebrianchen added the api: firestore label Jun 22, 2020

thebrianchen requested a review from egilmorez as a code owner June 22, 2020 23:32

thebrianchen self-assigned this Jun 22, 2020

add changelog

3927512

use a new doc each time to avoid flakes

1fd7d66

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Jun 23, 2020

thebrianchen requested a review from schmidt-sebastian June 23, 2020 03:03

schmidt-sebastian reviewed Jun 23, 2020

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Jun 23, 2020

delete extra d.ts methods

390d49c

thebrianchen requested a review from schmidt-sebastian June 24, 2020 19:01

thebrianchen assigned schmidt-sebastian and egilmorez and unassigned thebrianchen Jun 24, 2020

Brian Chen added 4 commits June 24, 2020 19:32

go back to overloads instead of union

4e8ca3c

typo

fd11cd7

Merge branch 'master' (early part) into bc/set-override

4b12457

fix test

767c936

Merge branch 'master' into bc/set-override

940c86d

thebrianchen requested review from Feiyang1, hiranya911 and hsubox76 as code owners June 25, 2020 18:30

thebrianchen force-pushed the bc/set-override branch 2 times, most recently from ec8c8e4 to 74aca83 Compare June 25, 2020 19:08

add changeset, remove changelog

89f2c8d

thebrianchen force-pushed the bc/set-override branch from 74aca83 to 89f2c8d Compare June 25, 2020 19:13

Feiyang1 reviewed Jun 25, 2020

View reviewed changes

.changeset/rare-rabbits-grab.md Outdated Show resolved Hide resolved

Brian Chen added 2 commits June 25, 2020 12:30

update changeset to include firebase

a09f5cf

Merge branch 'master' into bc/set-override

7dafd4a

thebrianchen mentioned this pull request Jun 25, 2020

Add set() overrides to lite sdk #3291

Merged

Merge branch 'master' into bc/set-override

bcc09ba

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Jun 25, 2020

Brian Chen added 2 commits June 25, 2020 21:40

propagate converter into error message

f289a76

remove unneeded import

695d0a4

thebrianchen force-pushed the bc/set-override branch from 74e7a77 to 695d0a4 Compare June 26, 2020 04:51

schmidt-sebastian approved these changes Jun 26, 2020

View reviewed changes

change param order

3c00f96

thebrianchen requested a review from Feiyang1 June 26, 2020 06:12

thebrianchen assigned Feiyang1 Jun 26, 2020

Feiyang1 removed their assignment Jun 26, 2020

egilmorez requested a review from markarndt June 26, 2020 17:18

thebrianchen assigned Feiyang1 and unassigned Feiyang1 Jun 26, 2020

markarndt reviewed Jun 26, 2020

View reviewed changes

egilmorez approved these changes Jun 26, 2020

View reviewed changes

add backticks

57ed84b

thebrianchen assigned Feiyang1 and unassigned egilmorez and thebrianchen Jun 26, 2020

Feiyang1 approved these changes Jun 26, 2020

View reviewed changes

thebrianchen merged commit 39ca8ec into master Jun 26, 2020

thebrianchen deleted the bc/set-override branch June 26, 2020 19:39

github-actions bot mentioned this pull request Jul 6, 2020

Version Packages #3357

Merged

firebase locked and limited conversation to collaborators Jul 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for set() with SetOptions when using FirestoreDataConverter #3254

add support for set() with SetOptions when using FirestoreDataConverter #3254

thebrianchen commented Jun 22, 2020

google-oss-bot commented Jun 22, 2020 •

edited

`@firebase/firestore`

`@firebase/firestore/exp`

`@firebase/firestore/lite`

`@firebase/firestore/memory`

`firebase`

schmidt-sebastian left a comment

schmidt-sebastian Jun 23, 2020

thebrianchen Jun 24, 2020

schmidt-sebastian Jun 23, 2020

thebrianchen Jun 24, 2020

schmidt-sebastian Jun 23, 2020

thebrianchen Jun 24, 2020

schmidt-sebastian Jun 23, 2020

thebrianchen Jun 24, 2020

schmidt-sebastian Jun 23, 2020

thebrianchen Jun 24, 2020

schmidt-sebastian Jun 25, 2020

thebrianchen Jun 26, 2020

schmidt-sebastian Jun 26, 2020

changeset-bot bot commented Jun 25, 2020 •

edited

schmidt-sebastian Jun 26, 2020

thebrianchen Jun 26, 2020

schmidt-sebastian Jun 26, 2020

thebrianchen Jun 26, 2020

schmidt-sebastian Jun 26, 2020

markarndt left a comment

markarndt Jun 26, 2020

thebrianchen Jun 26, 2020

egilmorez left a comment

egilmorez Jun 26, 2020

thebrianchen Jun 26, 2020

egilmorez Jun 26, 2020

thebrianchen Jun 26, 2020

add support for set() with SetOptions when using FirestoreDataConverter #3254

add support for set() with SetOptions when using FirestoreDataConverter #3254

Conversation

thebrianchen commented Jun 22, 2020

google-oss-bot commented Jun 22, 2020 • edited

Binary Size Report

Affected SDKs

@firebase/firestore

@firebase/firestore/exp

@firebase/firestore/lite

@firebase/firestore/memory

firebase

Test Logs

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Jun 25, 2020 • edited

🦋 Changeset is good to go

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markarndt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egilmorez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-oss-bot commented Jun 22, 2020 •

edited

`@firebase/firestore`

`@firebase/firestore/exp`

`@firebase/firestore/lite`

`@firebase/firestore/memory`

`firebase`

changeset-bot bot commented Jun 25, 2020 •

edited