Firestore: QoL improvements for converters #5268

thebrianchen · 2021-08-10T17:57:07Z

Fixes #4277.

Changes implemented:

Support for FieldValues when using a converter
Support for nested Partial fields when using a converter.
Support for UpdateData with converter.

changeset-bot · 2021-08-10T17:57:10Z

🦋 Changeset detected

Latest commit: 635c9e3

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

This PR includes changesets to release 3 packages

Name	Type
firebase	Major
@firebase/firestore	Major
@firebase/rules-unit-testing	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

github-actions · 2021-08-10T18:00:53Z

Changeset File Check ✅

No modified packages are missing from the changeset file.
No changeset formatting errors detected.

schmidt-sebastian

Roughly LGTM :)

schmidt-sebastian · 2021-08-11T02:56:30Z

packages/firestore/src/lite/reference.ts

@@ -48,6 +49,21 @@ export interface DocumentData {
  [field: string]: any;
 }

+type Primitive = string | number | boolean | bigint | undefined | null;
+// eslint-disable-next-line @typescript-eslint/ban-types
+type Builtin = Primitive | Function;


Do we use Function?

I used it when I had special FieldValue return types. Removed it.

packages/firestore/src/lite/reference.ts

google-oss-bot · 2021-08-12T22:13:03Z

Binary Size Report

Affected SDKs

`@firebase/firestore`

Type	Base (`cca8cdf`)	Head (`5301fc7`)	Diff
browser	285 kB	285 kB	-136 B (-0.0%)
esm2017	227 kB	227 kB	+28 B (+0.0%)
main	531 kB	531 kB	+54 B (+0.0%)
module	285 kB	285 kB	-136 B (-0.0%)
react-native	227 kB	227 kB	+28 B (+0.0%)

`@firebase/firestore-compat`

Type	Base (`cca8cdf`)	Head (`5301fc7`)	Diff
browser	29.0 kB	29.0 kB	+54 B (+0.2%)
main	38.1 kB	38.2 kB	+64 B (+0.2%)
module	29.0 kB	29.0 kB	+54 B (+0.2%)
react-native	28.7 kB	28.7 kB	+54 B (+0.2%)

`@firebase/firestore/bundle`

Type	Base (`cca8cdf`)	Head (`5301fc7`)	Diff
browser	291 kB	291 kB	+28 B (+0.0%)
esm2017	177 kB	177 kB	+28 B (+0.0%)
main	527 kB	528 kB	+54 B (+0.0%)
module	291 kB	291 kB	+28 B (+0.0%)
react-native	177 kB	177 kB	+28 B (+0.0%)

`@firebase/firestore/memory`

Type	Base (`cca8cdf`)	Head (`5301fc7`)	Diff
browser	217 kB	217 kB	+116 B (+0.1%)
esm2017	173 kB	173 kB	+28 B (+0.0%)
main	325 kB	325 kB	+54 B (+0.0%)
module	217 kB	217 kB	+116 B (+0.1%)
react-native	173 kB	173 kB	+28 B (+0.0%)

`@firebase/firestore/memory-bundle`

Type	Base (`cca8cdf`)	Head (`5301fc7`)	Diff
browser	225 kB	225 kB	+28 B (+0.0%)
esm2017	177 kB	177 kB	+28 B (+0.0%)
main	322 kB	322 kB	+54 B (+0.0%)
module	225 kB	225 kB	+28 B (+0.0%)
react-native	177 kB	177 kB	+28 B (+0.0%)

firebase

Type Base (cca8cdf) Head (5301fc7) Diff

firebase-firestore.js 337 kB 337 kB +28 B (+0.0%)

firebase-firestore.memory.js 271 kB 271 kB +28 B (+0.0%)

firebase.js 896 kB 896 kB +28 B (+0.0%)

Test Logs

Base (cca8cdff): https://github.com/firebase/firebase-js-sdk/actions/runs/1144169051
Head (5301fc7f): https://github.com/firebase/firebase-js-sdk/actions/runs/1144342583

google-oss-bot · 2021-08-12T22:18:33Z

Size Analysis Report

Affected Products

No changes between base commit (cca8cdf) and head commit (5301fc7).

Test Logs

schmidt-sebastian

I think this is great. Some cleanup suggestions, most of which you will probably push back on :)

schmidt-sebastian · 2021-08-17T15:10:55Z

common/api-review/firestore-lite.api.md

-    toFirestore(modelObject: T): DocumentData;
-    toFirestore(modelObject: Partial<T>, options: SetOptions): DocumentData;
+    toFirestore(modelObject: WithFieldValue<T>): DocumentData;
+    toFirestore(modelObject: NestedPartial<T>, options: SetOptions): DocumentData;


I thought we were going to align those two names. Have you thought of a naming pattern that would achieve that?

Renamed to PartialWithFieldValue.

packages/firebase/index.d.ts

schmidt-sebastian · 2021-08-17T15:12:39Z

packages/firestore/compat/config.ts

@@ -22,7 +22,7 @@ import { _FirebaseNamespace } from '@firebase/app-types/private';
 import { Component, ComponentType } from '@firebase/component';

 import {
-  FirebaseFirestore,
+  Firestore as FirebaseFirestore,


Intentional?

Yes. Not sure how this worked in the past, since FirebaseFirestore isn't exported by ../exp/index.

schmidt-sebastian · 2021-08-17T15:15:08Z

packages/firestore/exp/api.ts

+  NestedUpdateFields,
+  AddPrefixToKeys,
+  NestedPartial,
+  UnionToIntersection,


Optional: I think it would be cleaner if these were exports from a dedicated module, which would make it easier to understand that SetOptions and Primitive are completely unrelated.

Are you saying to move the helper types into a separate exp/helper.ts file, or for lite as well?

Something like lite/types.ts that you can refer to from exp/ and lite/

Moved into lite/types

schmidt-sebastian · 2021-08-17T15:15:37Z

packages/firestore/src/api/database.ts

@@ -452,9 +453,9 @@ export class Transaction implements PublicTransaction, Compat<ExpTransaction> {
    const ref = castReference(documentRef);
    if (options) {
      validateSetOptions('Transaction.set', options);
-      this._delegate.set(ref, data, options);
+      this._delegate.set(ref, data as NestedPartial<T>, options);


Are these needed?

Yes, since the method parameter types are T | Partial<T>, we have to cast.

schmidt-sebastian · 2021-08-17T15:27:10Z

packages/firestore/src/lite/user_data_reader.ts

@@ -73,8 +73,8 @@ const RESERVED_FIELD_REGEX = /^__.*__$/;
 * lite, firestore-exp and classic SDK.
 */
 export interface UntypedFirestoreDataConverter<T> {


Please also update the docs for FirestoreDataConverter to explain what this new input type is.

Added for UntypedFirestoreDataConverter and FirestoreDataConverter.

packages/firestore/test/lite/integration.test.ts

schmidt-sebastian · 2021-08-17T15:30:40Z

packages/firestore/test/lite/integration.test.ts

+          const ref = doc(collection(db, 'testobj')).withConverter(
+            testConverter
+          );
+          await setBaseDoc(ref);


There is also withTestDocAndInitialData that should give you an existing doc.

…c/types

schmidt-sebastian

LGTM

.changeset/dirty-pandas-pay.md

packages/firestore/src/exp/snapshot.ts

packages/firestore/src/lite/reference.ts

packages/firestore/src/lite/snapshot.ts

svailla4 · 2021-08-25T23:45:05Z

UpdateData is very nice, but is there a way to make dot notation work with optional fields? @thebrianchen
Example: link

thebrianchen · 2021-08-26T17:42:50Z

@svailla4 Thanks for catching this! Will look into this some more.

ishowta · 2021-08-28T18:45:37Z

This allows for dot notation in the middle of the object, but this probably won't work. link

type Schema = {
  foo: {
    bar: {
      baz: number
    }
  }
}

const updateData: UpdateData<Schema> = {
  foo: {
    "bar.baz": 3
  }
}

ishowta · 2021-08-28T19:23:30Z

I get an error when using this with Record in Typescript 4.4. link
This is probably due to the fact that the records are somehow appearing at the top level (regardless of the Typescript version).

type Schema = {
  foo: {
    bar: Record<string, string>
  }
}

const updateData: UpdateData<Schema> = {
  foo:{
    bar: {
      baz: 'hello'
    }
  }
}

thebrianchen added the api: firestore label Aug 10, 2021

thebrianchen self-assigned this Aug 10, 2021

WIP: working NestedParial and WithFieldValue

f5a00fc

thebrianchen force-pushed the bc/types branch from db4c5ea to f5a00fc Compare August 10, 2021 18:55

Brian Chen added 2 commits August 10, 2021 18:18

add support for TypedUpdateData + sanity tests

e87b617

add support for WriteBatch and Transaction

fdd432a

schmidt-sebastian reviewed Aug 11, 2021

View reviewed changes

incremental fixes

01e1529

thebrianchen force-pushed the bc/types branch from aecc8fa to 01e1529 Compare August 11, 2021 17:03

Brian Chen added 7 commits August 11, 2021 12:24

fix exports/imports for exp and lite

1d48049

Merge branch 'master' into bc/types

de10404

Fix docs and imports

d2df5e8

Fixing imports

21cc4bf

move ts-lint

c5bb26f

add comments and skip test

c2e0e38

trying fei's rollup hack

479b6bb

allow skip

1ffaad2

Brian Chen added 5 commits August 16, 2021 10:55

merge and rename

eb7d26c

revert rollup changes

b0568bb

add integration tests

826fff2

Merge branch 'master' into bc/types

d94b0f4

revert auth-exp lint

0b620ca

schmidt-sebastian reviewed Aug 17, 2021

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned thebrianchen Aug 17, 2021

resolve comments

da10a15

Brian Chen and others added 4 commits August 17, 2021 18:42

some comment changes

706c060

Update API reports

a8123d8

fix addDoc test

f9d4b3e

Merge branch 'bc/types' of github.com:firebase/firebase-js-sdk into b…

416850e

…c/types

thebrianchen requested a review from schmidt-sebastian August 18, 2021 15:29

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Aug 18, 2021

thebrianchen changed the title ~~Firestore: Prototyping for TS updates~~ Firestore: QoL improvements for converters Aug 18, 2021

schmidt-sebastian approved these changes Aug 18, 2021

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Aug 18, 2021

update comments

5a175a4

thebrianchen assigned Feiyang1 and unassigned thebrianchen Aug 18, 2021

Merge branch 'master' into bc/types

635c9e3

Feiyang1 approved these changes Aug 18, 2021

View reviewed changes

hsubox76 approved these changes Aug 18, 2021

View reviewed changes

thebrianchen merged commit 5bc6afb into master Aug 18, 2021

thebrianchen deleted the bc/types branch August 18, 2021 23:50

thebrianchen assigned thebrianchen and unassigned Feiyang1 Aug 18, 2021

schmidt-sebastian pushed a commit that referenced this pull request Aug 20, 2021

Firestore: QoL improvements for converters (#5268)

43138fa

thebrianchen mentioned this pull request Aug 24, 2021

feat!: add QoL improvements for converters googleapis/nodejs-firestore#1595

Merged

google-oss-bot mentioned this pull request Aug 24, 2021

Version Packages #5362

Merged

thebrianchen mentioned this pull request Aug 27, 2021

Support union types and optional fields with dot separation on UpdateData #5394

Merged

firebase locked and limited conversation to collaborators Sep 18, 2021

Firestore: QoL improvements for converters #5268

Firestore: QoL improvements for converters #5268

Conversation

thebrianchen commented Aug 10, 2021 • edited

changeset-bot bot commented Aug 10, 2021 • edited

🦋 Changeset detected

github-actions bot commented Aug 10, 2021 • edited

Changeset File Check ✅

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

google-oss-bot commented Aug 12, 2021 • edited

Binary Size Report

Affected SDKs

@firebase/firestore

@firebase/firestore-compat

@firebase/firestore/bundle

@firebase/firestore/memory

@firebase/firestore/memory-bundle

firebase

Test Logs

google-oss-bot commented Aug 12, 2021 • edited

Size Analysis Report

Affected Products

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

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

svailla4 commented Aug 25, 2021 • edited

thebrianchen commented Aug 26, 2021

ishowta commented Aug 28, 2021

ishowta commented Aug 28, 2021

thebrianchen commented Aug 10, 2021 •

edited

changeset-bot bot commented Aug 10, 2021 •

edited

github-actions bot commented Aug 10, 2021 •

edited

google-oss-bot commented Aug 12, 2021 •

edited

`@firebase/firestore`

`@firebase/firestore-compat`

`@firebase/firestore/bundle`

`@firebase/firestore/memory`

`@firebase/firestore/memory-bundle`

`firebase`

google-oss-bot commented Aug 12, 2021 •

edited

svailla4 commented Aug 25, 2021 •

edited