Refactor and remove object utility functions #1811

mmermerkaya · 2019-05-23T14:18:51Z

Used some newer JavaScript methods instead of custom implementations. Fixed types.

Also did the same things in packages that used the refactored object utility functions.

Feiyang1

Thank you very much for taking it up! The more types the better! We will fix more places once we enable the global tslint checks.

I think we should also update @firebase/polyfills to include object.values and object.entries.

It serves as a documentation for us
I'd like to be able to point developers to this package and say these are the es2015+ features we use. They can manually include @firebase/polyfills if they don't want to or couldn't use Babel. We won't recommend it since we won't know what developers use in their own code, but I think it can be a nice option for people who is upgrading from v5 to v6.

packages/database/src/core/PersistentConnection.ts

packages/util/src/obj.ts

mmermerkaya · 2019-05-23T18:38:13Z

@Feiyang1 No problem! Also LMK if you need any help with the TSLint stuff.

Added those two to polyfills.

I think that the polyfills package is going to be difficult to keep in sync unless we change lib in the base TSConfig to es5 and then add the libs one by one, which would cause an error if we use a function that's not included there. It's not possible to add every single method independently, but there's ES2017.object at least. So we'd add that to lib in TSConfig and then import 'core-js/features/object'; to the polyfill package.

It's fine like this for now though. Just an idea.

schmidt-sebastian

I love cleanup PRs! Thanks for doing this. I wonder if we can reduce our usage of the utility functions more and left some comments. Let me know what you think!

packages/database/src/core/PersistentConnection.ts

packages/database/src/core/snap/IndexMap.ts

packages/database/src/core/snap/Node.ts

packages/database/src/core/view/ChildChangeAccumulator.ts

schmidt-sebastian · 2019-05-24T05:35:35Z

packages/database/src/core/view/ChildChangeAccumulator.ts

@@ -41,44 +34,50 @@ export class ChildChangeAccumulator {
      childKey !== '.priority',
      'Only non-priority child changes can be tracked.'
    );
-    const oldChange = safeGet(this.changeMap_, childKey) as Change;
+    const oldChange = this.changeMap.get(childKey);


@mikelehen Do you know if there is a reason for safeGet()?. Are we guarding against queries for length or something similar?

I think it's just guarding against accidentally getting a value from the prototype chain.

packages/util/src/obj.ts

googlebot · 2019-05-24T22:35:27Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-25T01:15:52Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Feiyang1 · 2019-06-21T16:47:29Z

@mmermerkaya What is the state of the PR? any blocker?

schmidt-sebastian

I like the simplification a lot, but I do think that we should be careful about using the Object.keys/Object.values functions as they create copies of data. There are some parts of this PR where I would really advocate we don't do that (ImmutableTree for example), but in general I think it will be better if we directly iterate everywhere without using these helpers.

I flagged some usages here, some of them may be insignificant. I also left some out where I think we don't need to bother (like the StatsCollector).

packages/database/src/core/Repo.ts

packages/database/src/core/SyncPoint.ts

packages/database/src/core/snap/nodeFromJSON.ts

packages/database/src/core/util/ImmutableTree.ts

packages/database/src/core/util/Tree.ts

packages/database/src/core/util/validation.ts

mmermerkaya · 2019-06-25T14:42:08Z

Reverted (almost) all changes that added Object.* calls in Database. Replaced the forEach function from @firebase/util with DB's own util function each.

FWIW, the need to iterate over plain JS objects looks like a code smell to me. I'd recommend using ES6 Maps over objects if the keys aren't known in compile time and/or can change.

schmidt-sebastian

LGTM. I do want to ask @Feiyang1 about the Polyfills though, so please hold off on merging.

packages/database/src/core/PersistentConnection.ts

Feiyang1

Just 1 question and a pending PR #1922

packages/database/src/core/Repo.ts

* cherry pick es2015+ libs * remove lib flag from installations

mmermerkaya requested review from Feiyang1, hiranya911, hsubox76, jsdt, mikelehen and schmidt-sebastian as code owners May 23, 2019 14:18

mmermerkaya force-pushed the mmermerkaya-refactor-obj branch from fbdf492 to 83b368f Compare May 23, 2019 15:04

Feiyang1 reviewed May 23, 2019

View reviewed changes

Feiyang1 assigned schmidt-sebastian May 23, 2019

schmidt-sebastian reviewed May 24, 2019

View reviewed changes

schmidt-sebastian assigned mmermerkaya and unassigned schmidt-sebastian May 24, 2019

mmermerkaya force-pushed the mmermerkaya-refactor-obj branch from f5d190a to 407a59a Compare May 24, 2019 13:54

zijianjoy force-pushed the mmermerkaya-refactor-obj branch from 407a59a to eed06e8 Compare May 24, 2019 22:30

mmermerkaya requested review from alikn, andirayo, bojeil-google, davideast, dwoffinden, jamesdaniels, pinarx, ryanpbrewster, scottcrossen, wti806, yuchenshi and zijianjoy as code owners May 24, 2019 22:30

wilhuff force-pushed the mmermerkaya-refactor-obj branch from eed06e8 to 407a59a Compare May 25, 2019 01:15

schmidt-sebastian removed their assignment May 31, 2019

mmermerkaya force-pushed the mmermerkaya-refactor-obj branch from 69c6876 to 0c4594a Compare June 24, 2019 15:08

mmermerkaya changed the title ~~Refactor object utility functions~~ Refactor and remove object utility functions Jun 24, 2019

mmermerkaya assigned schmidt-sebastian and unassigned mmermerkaya Jun 24, 2019

schmidt-sebastian reviewed Jun 24, 2019

View reviewed changes

mmermerkaya force-pushed the mmermerkaya-refactor-obj branch from 17f9aa3 to 1ab4ad0 Compare June 25, 2019 14:40

schmidt-sebastian reviewed Jun 25, 2019

View reviewed changes

packages/database/src/core/PersistentConnection.ts Outdated Show resolved Hide resolved

schmidt-sebastian approved these changes Jun 25, 2019

View reviewed changes

schmidt-sebastian removed their assignment Jun 25, 2019

mmermerkaya added 11 commits June 26, 2019 18:41

Refactor object utility functions

4cd96bd

Remove some object utility functions

f0aaf82

Remove redundant comments

9c17a49

Address comments

3d57559

Fix tests and lint errors

dd5e95c

Revert DB changes

5610a11

Remove some redundant comments

8b15f1a

Fix tests

d1223a4

Remove comment

1999dc0

Add comment

fd696d6

Make map work with strict flag

082b90d

mmermerkaya force-pushed the mmermerkaya-refactor-obj branch from d2ec322 to 082b90d Compare June 26, 2019 17:52

Feiyang1 approved these changes Jun 26, 2019

View reviewed changes

packages/database/src/core/Repo.ts Show resolved Hide resolved

cherry pick es2015+ libs (#1922)

5b110bc

* cherry pick es2015+ libs * remove lib flag from installations

mmermerkaya merged commit 857b0fe into master Jun 27, 2019

mmermerkaya deleted the mmermerkaya-refactor-obj branch June 27, 2019 19:16

Miguel-Alberto mentioned this pull request Aug 6, 2019

undefined is not an object (evaluating ‘s.values’) #2015

Closed

firebase locked and limited conversation to collaborators Oct 11, 2019

Refactor and remove object utility functions #1811

Refactor and remove object utility functions #1811

Uh oh!

Conversation

mmermerkaya commented May 23, 2019

Uh oh!

Feiyang1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mmermerkaya commented May 23, 2019

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

schmidt-sebastian May 24, 2019

Choose a reason for hiding this comment

Uh oh!

mmermerkaya May 24, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

googlebot commented May 24, 2019

Uh oh!

googlebot commented May 25, 2019

Uh oh!

Feiyang1 commented Jun 21, 2019

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mmermerkaya commented Jun 25, 2019

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Feiyang1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!