Remove namespace import for objUtils #2807

schmidt-sebastian · 2020-03-26T22:40:44Z

This PR removes the namespace import for objUtils and replaces a bunch of usages of the helper functions with ES6 standard primitives (Objects -> Maps).

I verified that the object that I am now using Object.keys()/Object.values() on don't have any inherited properties.

google-oss-bot · 2020-03-26T22:47:54Z

Binary Size Report

Affected SDKs

SDK	Type	Base (`50de3fb`)	Head (`1bb1a67`)	Diff
@firebase/firestore/memory	browser	224500.00	224475.00	-25.00 (-0.01%)
	module	221680.00	221655.00	-25.00 (-0.01%)
	esm2017	170334.00	170304.00	-30.00 (-0.02%)
	main	397701.00	397404.00	-297.00 (-0.07%)
@firebase/firestore	browser	271073.00	271055.00	-18.00 (-0.01%)
	module	267852.00	267834.00	-18.00 (-0.01%)
	esm2017	216152.00	216141.00	-11.00 (-0.01%)
	main	488427.00	488418.00	-9.00 (-0.00%)
firebase	firebase.js	846912.00	846872.00	-40.00 (-0.00%)
	firebase-firestore.memory.js	266794.00	266750.00	-44.00 (-0.02%)
	firebase-firestore.js	312035.00	311995.00	-40.00 (-0.01%)

Metric Unit: byte

Test Logs

schmidt-sebastian · 2020-03-26T23:08:04Z

Better diff for viewing: #2807

dconeybe · 2020-03-30T14:13:06Z

packages/firestore/src/core/sync_engine.ts

@@ -912,7 +914,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
      const activeTargets: TargetId[] = [];

      let p = Promise.resolve();
-      objUtils.forEachNumber(this.queriesByTarget, (targetId, _) => {
+      this.queriesByTarget.forEach((_, targetId) => {


Could these forEach loops be replaced with simpler and, arguable, more readable,for loops?

e.g.

this.queriesByTarget.forEach((_, targetId) => {

could be changed to:

for (targetId of this.queriesByTarget.keys()) {

I tried making this work (I pushed two commits and one revert). The problem is that I would have to turn on "downlevelIteration" to make this work with ES5, which adds a lot of code (+4%).

The example you posted actually does not need downlevelIteration since you are converting the Map to an Array. That creates an extra copy though and hence I decided to leave this as is.

Cool. Thanks for looking into this.

dconeybe · 2020-03-30T14:15:52Z

packages/firestore/src/local/encoded_resource_path.ts

@@ -72,7 +72,7 @@ const encodedEscape = '\u0011';
 /**
 * Encodes a resource path into a IndexedDb-compatible string form.
 */
-export function encode(path: ResourcePath): EncodedResourcePath {
+export function encodeResourcePath(path: ResourcePath): EncodedResourcePath {


What is your rationale for renaming these functions? It's not that I disagree, it's just that the motivation is unclear to me.

The are now used as top-level functions. I renamed them to be more descriptive than just "encode/decode".

That's what I figured. Makes sense.

This reverts commit b04f425.

schmidt-sebastian · 2020-03-30T17:53:20Z

@wilhuff for approval.

dconeybe · 2020-03-30T17:57:40Z

packages/firestore/src/core/sync_engine.ts

@@ -912,7 +914,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
      const activeTargets: TargetId[] = [];

      let p = Promise.resolve();
-      objUtils.forEachNumber(this.queriesByTarget, (targetId, _) => {
+      this.queriesByTarget.forEach((_, targetId) => {


Cool. Thanks for looking into this.

dconeybe · 2020-03-30T17:57:56Z

packages/firestore/src/local/encoded_resource_path.ts

@@ -72,7 +72,7 @@ const encodedEscape = '\u0011';
 /**
 * Encodes a resource path into a IndexedDb-compatible string form.
 */
-export function encode(path: ResourcePath): EncodedResourcePath {
+export function encodeResourcePath(path: ResourcePath): EncodedResourcePath {


That's what I figured. Makes sense.

wilhuff · 2020-03-30T18:41:19Z

packages/firestore/src/platform_node/node_platform.ts

@@ -57,8 +57,8 @@ export class NodePlatform implements Platform {
  }

  formatJSON(value: unknown): string {
-    // util.inspect() results in much more readable output than JSON.stringify()
-    return util.inspect(value, { depth: 100 });
+    // util's inspect() results in much more readable output than JSON.stringify()


util.inspect previously matched JSON.stringify. Now these are inconsistent.

I don't follow this. Can you elaborate? JSON.stringify is a global and doesn't need to be (cannot be?) imported.

What I meant was that in the comment you have "util's inspect()" but "JSON.stringify()". Spelling the former as "util.inspect()" would keep the text references consistent.

Okay, got it. Fixed.

wilhuff · 2020-03-30T18:45:20Z

packages/firestore/src/remote/remote_event.ts

@@ -41,7 +41,7 @@ export class RemoteEvent {
    /**
     * A map from target to changes to the target. See TargetChange.
     */
-    readonly targetChanges: { [targetId: number]: TargetChange },
+    readonly targetChanges: Map<TargetId, TargetChange>,


I thought we avoided ES 2015 Map because it wasn't available everywhere. Has this changed?

Map is actually widely available, just some of the support for individual methods is lacking (and our solution for that are Polyfills).

Even IE11 understands Map for the most part: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map

We also use Map in one other place:

firebase-js-sdk/packages/firestore/src/model/field_value.ts

Line 142 in cae4645

type Overlay = Map<string, Overlay> | api.Value | null;

(recently introduced in the field value rewrite). I was actually surprised we don't use it in more places.

wilhuff · 2020-03-30T18:56:07Z

packages/firestore/src/remote/watch_change.ts

@@ -596,11 +595,11 @@ export class WatchChangeAggregator {
  }

  private ensureTargetState(targetId: TargetId): TargetState {
-    if (!this.targetStates[targetId]) {
-      this.targetStates[targetId] = new TargetState();
+    if (!this.targetStates.has(targetId)) {


You could make this a single Map access in the common case and avoid the force unwrap by rewriting this as:

let result = this.targetStates.get(targetId); if (result !== undefined) { result = new TargetState(); this.targetStates.set(targetId, result); } return result;

wilhuff · 2020-03-30T19:01:30Z

packages/firestore/src/api/database.ts

-    if (
-      objUtils.contains(settingsLiteral as objUtils.Dict<{}>, 'persistence')
-    ) {
+    if (settingsLiteral.hasOwnProperty('persistence')) {


If the user has overridden hasOwnProperty this validation result cannot be trusted. Also, if the object was created with Object.create(null) the object has no prototype and this invocation will throw. Calling Object.prototype.hasOwnProperty.call is the safer way to phrase this. Since we do this in a few places and the reference can't be minified, it's probably cheaper to keep the contains method.

Since this is really validation-related rather than general object manipulation it could make sense to move contains into input_validation.ts.

Fixed. The only usage that is sensible is in this file, so I moved contains here.

wilhuff · 2020-03-30T19:07:10Z

packages/firestore/test/unit/local/web_storage_shared_client_state.test.ts

@@ -103,7 +103,7 @@ class TestSharedClientSyncer implements SharedClientStateSyncer {

  get sharedClientState(): TestSharedClientState {
    return {
-      mutationCount: objUtils.size(this.mutationState),
+      mutationCount: size(this.mutationState),


This is a really ambiguous name now. Maybe objectSize?

wilhuff · 2020-03-30T19:10:13Z

packages/firestore/test/unit/specs/spec_builder.ts

@@ -442,14 +438,14 @@ export class SpecBuilder {

    // Clear any preexisting limbo watch targets, which we'll re-create as
    // necessary from the provided keys below.
-    objUtils.forEach(this.limboMapping, (key, targetId) => {
+    forEach(this.limboMapping, (key, targetId) => {


You could map limboMapping a Map as well.

There are a lot of data structures the spec tests that can be converted - but not all of them (since some of them are used in the JSON output). For my sanity, I decided to only clean up the types that relied on the "utils" methods that I removed.

wilhuff · 2020-03-30T19:11:36Z

packages/firestore/test/unit/specs/spec_test_runner.ts

-  private expectedActiveTargets: {
-    [targetId: number]: { queries: SpecQuery[]; resumeToken: string };
-  };
+  private expectedActiveTargets: Map<


This type is repeated several times in here. Would it make sense to create a type alias for the map type or even just the value type within the map?

Updated here and in another place (used the key only as it is more widely used).

wilhuff

LGTM

wilhuff · 2020-03-31T17:46:48Z

packages/firestore/src/platform_node/node_platform.ts

@@ -57,8 +57,8 @@ export class NodePlatform implements Platform {
  }

  formatJSON(value: unknown): string {
-    // util.inspect() results in much more readable output than JSON.stringify()
-    return util.inspect(value, { depth: 100 });
+    // util's inspect() results in much more readable output than JSON.stringify()


What I meant was that in the comment you have "util's inspect()" but "JSON.stringify()". Spelling the former as "util.inspect()" would keep the text references consistent.

schmidt-sebastian added 2 commits March 26, 2020 15:40

Remove namespace import for objUtils

cd6cf88

[AUTOMATED]: License Headers

0778cf7

schmidt-sebastian requested a review from dconeybe March 26, 2020 23:08

schmidt-sebastian assigned dconeybe Mar 26, 2020

schmidt-sebastian added the api: firestore label Mar 26, 2020

schmidt-sebastian assigned dconeybe and unassigned dconeybe Mar 26, 2020

schmidt-sebastian added 3 commits March 27, 2020 11:29

Merge

401e974

Remove remaining namespace imports

a6c63c3

[AUTOMATED]: License Headers

bccccb5

dconeybe approved these changes Mar 30, 2020

View reviewed changes

dconeybe assigned schmidt-sebastian and unassigned dconeybe Mar 30, 2020

schmidt-sebastian added 2 commits March 30, 2020 10:27

Try downlevelIteration

b04f425

Revert "Try downlevelIteration"

4ebfb3d

This reverts commit b04f425.

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Mar 30, 2020

dconeybe approved these changes Mar 30, 2020

View reviewed changes

wilhuff reviewed Mar 30, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Mar 30, 2020

schmidt-sebastian added 3 commits March 30, 2020 21:41

Merge

3c9c4d2

Add contains

dead3b5

Review

336c7c7

schmidt-sebastian force-pushed the mrschmidt/inline branch from 65cdcac to 336c7c7 Compare March 31, 2020 05:37

Lint

47743c2

schmidt-sebastian removed their assignment Mar 31, 2020

schmidt-sebastian assigned wilhuff and schmidt-sebastian and unassigned schmidt-sebastian Mar 31, 2020

wilhuff approved these changes Mar 31, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Mar 31, 2020

Fix comment

4aeb466

schmidt-sebastian merged commit 8e70e4a into master Mar 31, 2020

schmidt-sebastian deleted the mrschmidt/inline branch April 6, 2020 19:26

firebase locked and limited conversation to collaborators May 1, 2020

Remove namespace import for objUtils #2807

Remove namespace import for objUtils #2807

Uh oh!

Conversation

schmidt-sebastian commented Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-oss-bot commented Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

Test Logs

Uh oh!

schmidt-sebastian commented Mar 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian commented Mar 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

schmidt-sebastian commented Mar 26, 2020 •

edited

Loading

google-oss-bot commented Mar 26, 2020 •

edited

Loading