Don't normalize Proto data during deepClone #5147

schmidt-sebastian · 2021-07-14T16:58:38Z

The SDK normalizes all Proto data into ProtobufJS before operating on them. This means that internally we use { seconds, nanos }. When we send data via WebChannel, we need to use Proto3 JSON. Unfortunately, in one of the code paths we needlessly normalize the Date data and storing it in a document.

Fixes #5047

changeset-bot · 2021-07-14T16:58:41Z

🦋 Changeset detected

Latest commit: 90f8aa0

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

This PR includes changesets to release 3 packages

Name	Type
firebase	Patch
@firebase/firestore	Patch
@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-07-14T17:01:31Z

Changeset File Check ⚠️

Changeset formatting error in following file:

Package "firebase-compat-typings-test" must depend on the current version of "firebase-exp": "9.0.0-beta.6" vs "file:../../packages-exp/firebase-exp"
Package "firebase-compat-typings-test" must depend on the current version of "firebase-exp": "9.0.0-beta.6" vs "file:../../packages-exp/firebase-exp"
Error: could not parse changeset - invalid frontmatter: ---
@firebase/storage: minor
---

Add NodeJS support to Cloud Storage for Firebase. This release changes the `main` field in package.json to point to a Node specific build. If you are building a bundle for borwser usage, please make sure that your bundler uses the `browser` field (the default).

    at Object.parseChangesetFile [as default] (/home/runner/work/firebase-js-sdk/firebase-js-sdk/node_modules/@changesets/parse/dist/parse.cjs.dev.js:35:11)
    at /home/runner/work/firebase-js-sdk/firebase-js-sdk/node_modules/@changesets/read/dist/read.cjs.dev.js:137:71
    at async Promise.all (index 0)
    at async Object.getChangesets [as default] (/home/runner/work/firebase-js-sdk/firebase-js-sdk/node_modules/@changesets/read/dist/read.cjs.dev.js:141:48)
    at async Object.getReleasePlan [as default] (/home/runner/work/firebase-js-sdk/firebase-js-sdk/node_modules/@changesets/get-release-plan/dist/get-release-plan.cjs.dev.js:70:22)
    at async getStatus (/home/runner/work/firebase-js-sdk/firebase-js-sdk/node_modules/@changesets/cli/dist/cli.cjs.dev.js:972:23)
    at async run$1 (/home/runner/work/firebase-js-sdk/firebase-js-sdk/node_modules/@changesets/cli/dist/cli.cjs.dev.js:1227:11)

…-sdk into mrschmidt/deepclone

google-oss-bot · 2021-07-14T17:19:09Z

Binary Size Report

Affected SDKs

@firebase/database

Type Base (7a340da) Head (d9d1149) Diff

browser 296 kB 296 kB +31 B (+0.0%)

esm2017 265 kB 265 kB +33 B (+0.0%)

main 299 kB 299 kB +31 B (+0.0%)

module 296 kB 296 kB +31 B (+0.0%)
@firebase/database-compat

Type Base (7a340da) Head (d9d1149) Diff

browser 86.5 kB 86.6 kB +33 B (+0.0%)

main 103 kB 103 kB +31 B (+0.0%)

module 86.5 kB 86.6 kB +33 B (+0.0%)
@firebase/database-exp

Type Base (7a340da) Head (d9d1149) Diff

browser 246 kB 246 kB +33 B (+0.0%)

main 278 kB 278 kB +31 B (+0.0%)

module 246 kB 246 kB +33 B (+0.0%)

`@firebase/firestore`

Type	Base (`7a340da`)	Head (`d9d1149`)	Diff
browser	284 kB	284 kB	+32 B (+0.0%)
esm2017	226 kB	226 kB	+32 B (+0.0%)
main	531 kB	531 kB	+21 B (+0.0%)
module	284 kB	284 kB	+32 B (+0.0%)
react-native	226 kB	227 kB	+32 B (+0.0%)

@firebase/firestore-exp

Type Base (7a340da) Head (d9d1149) Diff

browser 224 kB 224 kB +31 B (+0.0%)

main 510 kB 510 kB +21 B (+0.0%)

module 224 kB 224 kB +31 B (+0.0%)

react-native 224 kB 224 kB +31 B (+0.0%)

`@firebase/firestore-lite`

Type	Base (`7a340da`)	Head (`d9d1149`)	Diff
browser	72.5 kB	72.6 kB	+31 B (+0.0%)
main	149 kB	149 kB	+21 B (+0.0%)
module	72.5 kB	72.6 kB	+31 B (+0.0%)
react-native	72.8 kB	72.8 kB	+31 B (+0.0%)

`@firebase/firestore/bundle`

Type	Base (`7a340da`)	Head (`d9d1149`)	Diff
browser	290 kB	290 kB	+32 B (+0.0%)
esm2017	177 kB	177 kB	+32 B (+0.0%)
main	527 kB	527 kB	+21 B (+0.0%)
module	290 kB	290 kB	+32 B (+0.0%)
react-native	177 kB	177 kB	+32 B (+0.0%)

`@firebase/firestore/memory`

Type	Base (`7a340da`)	Head (`d9d1149`)	Diff
browser	216 kB	216 kB	+32 B (+0.0%)
esm2017	172 kB	172 kB	+32 B (+0.0%)
main	325 kB	325 kB	+21 B (+0.0%)
module	216 kB	216 kB	+32 B (+0.0%)
react-native	172 kB	172 kB	+32 B (+0.0%)

`@firebase/firestore/memory-bundle`

Type	Base (`7a340da`)	Head (`d9d1149`)	Diff
browser	224 kB	224 kB	+32 B (+0.0%)
esm2017	177 kB	177 kB	+32 B (+0.0%)
main	322 kB	322 kB	+21 B (+0.0%)
module	224 kB	224 kB	+32 B (+0.0%)
react-native	177 kB	177 kB	+32 B (+0.0%)

@firebase/storage

Type Base (7a340da) Head (d9d1149) Diff

main 64.1 kB 55.4 kB -8.70 kB (-13.6%)

`firebase`

Type	Base (`7a340da`)	Head (`d9d1149`)	Diff
firebase-database.js	187 kB	187 kB	+21 B (+0.0%)
firebase-firestore.js	336 kB	336 kB	+31 B (+0.0%)
firebase-firestore.memory.js	271 kB	271 kB	+31 B (+0.0%)
firebase.js	893 kB	893 kB	+52 B (+0.0%)

Test Logs

Base (7a340da8): https://github.com/firebase/firebase-js-sdk/actions/runs/1028255975
Head (d9d11499): https://github.com/firebase/firebase-js-sdk/actions/runs/1032440304

google-oss-bot · 2021-07-14T17:25:28Z

Size Analysis Report

Affected Products

@firebase/auth-exp
- RecaptchaVerifier
  Size Table
  Type Base (7a340da) Head (d9d1149) Diff
  size
  35.8 kB
  35.8 kB
  +51 B (+0.1%)
  size-with-ext-deps
  47.7 kB
  47.7 kB
  +52 B (+0.1%)
- getAuth
  Size Table
  Type Base (7a340da) Head (d9d1149) Diff
  size
  60.8 kB
  60.9 kB
  +51 B (+0.1%)
  size-with-ext-deps
  73.0 kB
  73.1 kB
  +52 B (+0.1%)
- browserPopupRedirectResolver
  Size Table
  Type Base (7a340da) Head (d9d1149) Diff
  size
  53.2 kB
  53.3 kB
  +51 B (+0.1%)
  size-with-ext-deps
  65.3 kB
  65.4 kB
  +52 B (+0.1%)

Test Logs

thebrianchen

lgtm, since I'll be OOO. I see a failing transaction JSON test, which could be related to this change.

thebrianchen · 2021-07-14T23:25:38Z

.changeset/thirty-suits-switch.md

+"@firebase/firestore": patch
+---
+
+Fixes an issue that prevented Timestamps to be used via `update()` when connected to the Emulator


nit: s/Fixes/Fixed, s/to be/from being

thebrianchen · 2021-07-14T23:27:29Z

packages/firestore/src/exp/database.ts

@@ -15,12 +15,12 @@
 * limitations under the License.
 */

+// eslint-disable-next-line import/no-extraneous-dependencies


any reason to move this?

It's now causing a Lint failure (some tools have been updated)

thebrianchen · 2021-07-14T23:35:57Z

packages/firestore/src/model/values.ts

-    return { timestampValue: { ...normalizeTimestamp(source.timestampValue) } };
+  } else if (
+    source.timestampValue &&
+    typeof source.timestampValue === 'object'


I don't follow why we need the typeof check in the else if block. In what conditions does timestampValue not have type object? Do we need an separate condition to handle that, or does returning {...source} in the else statement work?

The timestampValue can be a string, in which case {...} does not work. It would create an array of characters.

schmidt-sebastian

Thanks for the review. The failing test is from RTDB, but thanks for checking.

schmidt-sebastian · 2021-07-15T02:38:29Z

packages/firestore/src/exp/database.ts

@@ -15,12 +15,12 @@
 * limitations under the License.
 */

+// eslint-disable-next-line import/no-extraneous-dependencies


It's now causing a Lint failure (some tools have been updated)

schmidt-sebastian · 2021-07-15T02:38:33Z

.changeset/thirty-suits-switch.md

+"@firebase/firestore": patch
+---
+
+Fixes an issue that prevented Timestamps to be used via `update()` when connected to the Emulator


schmidt-sebastian · 2021-07-15T02:40:16Z

packages/firestore/src/model/values.ts

-    return { timestampValue: { ...normalizeTimestamp(source.timestampValue) } };
+  } else if (
+    source.timestampValue &&
+    typeof source.timestampValue === 'object'


The timestampValue can be a string, in which case {...} does not work. It would create an array of characters.

Don't normalize Proto data during deepClone

0c273f2

schmidt-sebastian requested review from Feiyang1, hiranya911 and hsubox76 as code owners July 14, 2021 16:58

Create thirty-suits-switch.md

4c0cba4

schmidt-sebastian requested a review from egilmorez as a code owner July 14, 2021 17:01

schmidt-sebastian added 2 commits July 14, 2021 11:03

Undo rollup change

f809262

Merge branch 'mrschmidt/deepclone' of github.com:firebase/firebase-js…

e143e9a

…-sdk into mrschmidt/deepclone

schmidt-sebastian assigned thebrianchen Jul 14, 2021

schmidt-sebastian added the api: firestore label Jul 14, 2021

thebrianchen approved these changes Jul 14, 2021

View reviewed changes

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen Jul 14, 2021

Update thirty-suits-switch.md

90f8aa0

schmidt-sebastian commented Jul 15, 2021

View reviewed changes

schmidt-sebastian merged commit 2cd9d7c into master Jul 15, 2021

schmidt-sebastian deleted the mrschmidt/deepclone branch July 15, 2021 16:11

google-oss-bot mentioned this pull request Jul 20, 2021

Version Packages #5177

Merged

firebase locked and limited conversation to collaborators Aug 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't normalize Proto data during deepClone #5147

Don't normalize Proto data during deepClone #5147

schmidt-sebastian commented Jul 14, 2021 •

edited

Loading

changeset-bot bot commented Jul 14, 2021 •

edited

Loading

github-actions bot commented Jul 14, 2021 •

edited

Loading

google-oss-bot commented Jul 14, 2021 •

edited

Loading

`@firebase/database`

`@firebase/database-compat`

`@firebase/database-exp`

`@firebase/firestore`

`@firebase/firestore-exp`

`@firebase/firestore-lite`

`@firebase/firestore/bundle`

`@firebase/firestore/memory`

`@firebase/firestore/memory-bundle`

`@firebase/storage`

`firebase`

google-oss-bot commented Jul 14, 2021 •

edited

Loading

Affected Products

`@firebase/auth-exp`

`RecaptchaVerifier`

`getAuth`

`browserPopupRedirectResolver`

Test Logs

thebrianchen left a comment

thebrianchen Jul 14, 2021

schmidt-sebastian Jul 15, 2021

thebrianchen Jul 14, 2021

schmidt-sebastian Jul 15, 2021

thebrianchen Jul 14, 2021

schmidt-sebastian Jul 15, 2021

schmidt-sebastian left a comment

schmidt-sebastian Jul 15, 2021

schmidt-sebastian Jul 15, 2021

schmidt-sebastian Jul 15, 2021

Don't normalize Proto data during deepClone #5147

Don't normalize Proto data during deepClone #5147

Conversation

schmidt-sebastian commented Jul 14, 2021 • edited Loading

changeset-bot bot commented Jul 14, 2021 • edited Loading

🦋 Changeset detected

github-actions bot commented Jul 14, 2021 • edited Loading

Changeset File Check ⚠️

google-oss-bot commented Jul 14, 2021 • edited Loading

Binary Size Report

Affected SDKs

@firebase/database

@firebase/database-compat

@firebase/database-exp

@firebase/firestore

@firebase/firestore-exp

@firebase/firestore-lite

@firebase/firestore/bundle

@firebase/firestore/memory

@firebase/firestore/memory-bundle

@firebase/storage

firebase

Test Logs

google-oss-bot commented Jul 14, 2021 • edited Loading

Size Analysis Report

Affected Products

@firebase/auth-exp

RecaptchaVerifier

getAuth

browserPopupRedirectResolver

Test Logs

thebrianchen 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

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

schmidt-sebastian commented Jul 14, 2021 •

edited

Loading

changeset-bot bot commented Jul 14, 2021 •

edited

Loading

github-actions bot commented Jul 14, 2021 •

edited

Loading

google-oss-bot commented Jul 14, 2021 •

edited

Loading

`@firebase/database`

`@firebase/database-compat`

`@firebase/database-exp`

`@firebase/firestore`

`@firebase/firestore-exp`

`@firebase/firestore-lite`

`@firebase/firestore/bundle`

`@firebase/firestore/memory`

`@firebase/firestore/memory-bundle`

`@firebase/storage`

`firebase`

google-oss-bot commented Jul 14, 2021 •

edited

Loading

`@firebase/auth-exp`

`RecaptchaVerifier`

`getAuth`

`browserPopupRedirectResolver`