refactor(database): classes for typescript database implementation #55

jsayol · 2017-06-15T21:05:26Z

Related to PRs #61 & #62

jsayol · 2017-06-15T21:08:20Z

src/database/core/view/EventRegistration.ts

+            );
+          } else {
+            // Exact match on each key.
+            return this.callbacks_.every(<any>function (cb, eventType) {


@jshcrowthe is the SDK using some library that adds the every method to Object.prototype? Otherwise, this looks like a bug to me.

In case it is indeed a bug, we could add this function to src/utils/obj.ts:

export const every = function(obj, fn) { for (let key in obj) { if (Object.prototype.hasOwnProperty.call(obj, key)) { if (!fn(key, obj[key])) { return false; } } } return true; };

and then replace this block of code with:

return every(this.callbacks_, (eventType, cb) => other.callbacks_[eventType] === cb);

Let me know! :)

I'm reviewing this at 3 AM but triggering this code proves you right. If we can add the helper fxn and update that block that'd be fantastic.

Josh, go get some sleep!

I'll update this as soon as I get home :)

googlebot · 2017-06-16T11:49:16Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot · 2017-06-16T12:40:38Z

CLAs look good, thanks!

googlebot · 2017-06-16T17:15:46Z

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 by someone other than the pull request submitter. We need to confirm that they're okay 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 the commit author(s) and merge this pull request when appropriate.

jsayol · 2017-06-16T17:19:52Z

I think I just offended the Git gods somehow... let me try to fix this.

Edit: phew, we're back on track.

googlebot · 2017-06-16T17:54:27Z

CLAs look good, thanks!

jsayol · 2017-06-16T18:16:59Z

That's all the classes done so feel free to review the changes.

jshcrowthe · 2017-06-16T18:36:47Z

Perfect. Thanks a ton @jsayol! With how large the database-ts branch is becoming, I am probably going to break things up a little. I'll get back to you with where this PR should be pointed.

jshcrowthe · 2017-06-16T19:38:00Z

@jsayol If you can please point this PR to https://github.com/firebase/firebase-js-sdk/tree/ts-database/add-new-impl (which will get incrementally merged in #62) then we can get this tested and merged.

jsayol · 2017-06-16T19:41:53Z

Sure. Should I tag it as [Part 3/3]?

jsayol · 2017-06-16T19:48:16Z

Yeah, Github didn't like it when I changed the base branch. I might have to rebase, one sec.

Edit: done.

jshcrowthe · 2017-06-16T20:29:16Z

I actually have another part 3 of my own, this will be a piece of part 2 😄

jsayol · 2017-06-16T20:30:03Z

Ah! Sorry, jumped the gun there :P

jshcrowthe

First look at this is really good. I have been left some comments on the Error/Closure comment style that if you could address would be awesome!

Thank you much for the typescript refactoring! It looks nice so far.

jshcrowthe · 2017-06-16T20:35:19Z

src/database/api/DataSnapshot.ts

-   * @private
-   * @const
-   * @type {!fb.core.snap.Node}
+   * @param {!fb.core.snap.Node} node_ A SnapshotNode to wrap.


Can we refactor the fb.*.*.* comments? These all have clear analogs in the classes that we reference.

jshcrowthe · 2017-06-16T20:40:06Z

src/database/api/onDisconnect.ts

+   * @return {!firebase.Promise}
+   */
+  cancel(opt_onComplete) {
+    validateArgCount('firebase.database.onDisconnect().cancel', 0, 1, arguments.length);


Can we change these validate identifiers to

validateArgCount('OnDisconnect.cancel', 0, 1, arguments.length);

I feel like with this being a class we should validate in the same way

Same principle for all of these below

Sure. This originally said Firebase.onDisconnect().cancel so I changed the Firebase part to firebase.database. I've done the same in some other classes too, so just to be clear: in general should I remove the Firebase/firebase.database prefix and simply use the ClassName.methodName format?

Yeah, with these classes exposed, directly referencing the classes should be fine.

I've addressed both comments, plus a few other things I've found along the way.

I pushed these changes on a separate commit to make them easier to review, let me know if you want me to squash them before merging.

jshcrowthe · 2017-06-17T01:49:28Z

src/database/core/ReadonlyRestClient.ts

    // no-op since we just always call getToken.
  }

  /** @inheritDoc */
-  onDisconnectPut(pathString, data, opt_onComplete) { }
+  onDisconnectPut(pathString: string, data: any, onComplete?: (a: string, b: string) => any) { }


It seems like converting ServerActions to an abstract base class with default implementations for these methods would probably remove the need for the empty methods. I don't think it should block this PR but something to do in the future.

Ah, that's a good point. I'll create a new PR against ts-database/add-new-impl with that change, thanks!

jshcrowthe · 2017-06-17T02:09:32Z

I've tested this and things are looking fantastic. I'm in the process of fixing node support and then importing tests so hopefully we can get all of these changes into master soon!

* refactor(database): classes for typescript database implementation * refactor(database): requested changes & other improvements

@schmidt-sebastian

…#62) * refactor(database): add typescript implementation * refactor(database): update build process to include database.ts All integration tests pass at this point * refactor(*): refactor environment builds to be based on separate .ts files * WIP: patch database code in nodeJS * refactor(database): classes for typescript database implementation (#55) * refactor(database): classes for typescript database implementation * refactor(database): requested changes & other improvements * fix(database): Add missing "typeof" (#74) https://github.com/firebase/firebase-js-sdk/blob/fd0728138d88c454f8e38a78f35d831d6365070c/src/database/js-client/core/Repo.js#L86 * WIP: fixes from @schmidt-sebastian's review * WIP: fix: TS Build error * fix(database): fix issue with missing repo method * WIP: review adjustments #1 * WIP: review comments #2

…irebase#55) * refactor(database): classes for typescript database implementation * refactor(database): requested changes & other improvements

* refactor(database): add typescript implementation * refactor(database): update build process to include database.ts All integration tests pass at this point * refactor(*): refactor environment builds to be based on separate .ts files * WIP: patch database code in nodeJS * refactor(database): classes for typescript database implementation (#55) * refactor(database): classes for typescript database implementation * refactor(database): requested changes & other improvements * WIP: add the /tests/config dir to the .gitignore * WIP: add test harness * WIP: add query tests There are some tests that have weird timing issues, and because of the polling nature of the original implementation, we never caught the issue. These should be resolved when able * WIP: add database.test.ts * WIP: add node.test.ts * WIP: add sortedmap.test.ts * WIP: add datasnapshot.test.ts * WIP: add sparsesnapshottree.test.ts * refactor(database): refactor query.test.ts to better preserve original test meaning * WIP: add crawler_support.test.ts * WIP: refactor EventAccumulator.ts for data.test.ts * WIP: fix issue with query.test.ts * WIP: add connection.test.ts * WIP: add info.test.ts * WIP: add order_by.test.ts I only migrated some of these tests as there was a dependency on the server for several tests * WIP: fix several code signature problems, add test files * WIP: add transaction.test.ts * WIP: working on the broken npm test command * WIP: working on fixes * WIP: remove logging * WIP: fix node tests * fix(*): fixing test files and CI integration * WIP: tEMP: Allow branch builds * WIP: escape string * refactor(CI): use ChromeHeadless launcher * WIP: fixes from review. * WIP: skip flakey test * WIP: remove unneeded debugger statement * WIP: fixing nits

@schmidt-sebastian

* refactor(database): remove old database implementation [Part 1/3] (#61) * refactor(database): remove old database implementation This is part #1 of 4 PR's to migrate database * refactor(database): remove database build processes * refactor(database): Add typescript database implementation [Part 2/3] (#62) * refactor(database): add typescript implementation * refactor(database): update build process to include database.ts All integration tests pass at this point * refactor(*): refactor environment builds to be based on separate .ts files * WIP: patch database code in nodeJS * refactor(database): classes for typescript database implementation (#55) * refactor(database): classes for typescript database implementation * refactor(database): requested changes & other improvements * fix(database): Add missing "typeof" (#74) https://github.com/firebase/firebase-js-sdk/blob/fd0728138d88c454f8e38a78f35d831d6365070c/src/database/js-client/core/Repo.js#L86 * WIP: fixes from @schmidt-sebastian's review * WIP: fix: TS Build error * fix(database): fix issue with missing repo method * WIP: review adjustments #1 * WIP: review comments #2 * WIP: refactor(database): Add migrated test harness [Part 3/3] (#71) * refactor(database): add typescript implementation * refactor(database): update build process to include database.ts All integration tests pass at this point * refactor(*): refactor environment builds to be based on separate .ts files * WIP: patch database code in nodeJS * refactor(database): classes for typescript database implementation (#55) * refactor(database): classes for typescript database implementation * refactor(database): requested changes & other improvements * WIP: add the /tests/config dir to the .gitignore * WIP: add test harness * WIP: add query tests There are some tests that have weird timing issues, and because of the polling nature of the original implementation, we never caught the issue. These should be resolved when able * WIP: add database.test.ts * WIP: add node.test.ts * WIP: add sortedmap.test.ts * WIP: add datasnapshot.test.ts * WIP: add sparsesnapshottree.test.ts * refactor(database): refactor query.test.ts to better preserve original test meaning * WIP: add crawler_support.test.ts * WIP: refactor EventAccumulator.ts for data.test.ts * WIP: fix issue with query.test.ts * WIP: add connection.test.ts * WIP: add info.test.ts * WIP: add order_by.test.ts I only migrated some of these tests as there was a dependency on the server for several tests * WIP: fix several code signature problems, add test files * WIP: add transaction.test.ts * WIP: working on the broken npm test command * WIP: working on fixes * WIP: remove logging * WIP: fix node tests * fix(*): fixing test files and CI integration * WIP: tEMP: Allow branch builds * WIP: escape string * refactor(CI): use ChromeHeadless launcher * WIP: fixes from review. * WIP: skip flakey test * WIP: remove unneeded debugger statement * WIP: fixing nits * Prevent using uninitialized array in EventEmitter (#85) * perf(*): fixing build size output issues * chore(*): remove unneeded build deps

jsayol commented Jun 15, 2017

View reviewed changes

jsayol force-pushed the db-classes branch 2 times, most recently from ff6727c to 1e51f99 Compare June 16, 2017 12:40

jsayol force-pushed the db-classes branch from 1e51f99 to 521e26a Compare June 16, 2017 12:50

jshcrowthe force-pushed the database-ts branch from bd03a98 to 6404ff2 Compare June 16, 2017 16:29

jsayol force-pushed the db-classes branch from e8a4253 to 1b53891 Compare June 16, 2017 17:54

jsayol force-pushed the db-classes branch 2 times, most recently from 327ee91 to 9afcadf Compare June 16, 2017 18:01

jsayol changed the title ~~feat(database): Convert several ES5 classes to TS classes~~ refactor(database): classes for typescript database implementation [Part 3/3] Jun 16, 2017

jsayol changed the base branch from database-ts to ts-database/add-new-impl June 16, 2017 19:46

refactor(database): classes for typescript database implementation

f0a7540

jsayol force-pushed the db-classes branch from 9afcadf to f0a7540 Compare June 16, 2017 19:51

jshcrowthe changed the title ~~refactor(database): classes for typescript database implementation [Part 3/3]~~ refactor(database): classes for typescript database implementation Jun 16, 2017

jshcrowthe self-assigned this Jun 16, 2017

jshcrowthe suggested changes Jun 16, 2017

View reviewed changes

jsayol added 2 commits June 17, 2017 01:30

refactor(database): requested changes & other improvements

8287d24

refactor(database): merge upstream

c5ebc31

jshcrowthe reviewed Jun 17, 2017

View reviewed changes

jshcrowthe approved these changes Jun 17, 2017

View reviewed changes

jshcrowthe merged commit 7efa1b0 into firebase:ts-database/add-new-impl Jun 17, 2017

jsayol deleted the db-classes branch June 17, 2017 08:26

jshcrowthe pushed a commit that referenced this pull request Jun 20, 2017

refactor(database): classes for typescript database implementation (#55)

2f6a406

* refactor(database): classes for typescript database implementation * refactor(database): requested changes & other improvements

jshcrowthe pushed a commit that referenced this pull request Jun 20, 2017

refactor(database): classes for typescript database implementation (#55)

56cde3c

* refactor(database): classes for typescript database implementation * refactor(database): requested changes & other improvements

firebase locked and limited conversation to collaborators Oct 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(database): classes for typescript database implementation #55

refactor(database): classes for typescript database implementation #55

jsayol commented Jun 15, 2017 •

edited

Loading

jsayol Jun 15, 2017

jsayol Jun 15, 2017

jshcrowthe Jun 16, 2017 •

edited

Loading

jsayol Jun 16, 2017

jsayol Jun 16, 2017

googlebot commented Jun 16, 2017

googlebot commented Jun 16, 2017

googlebot commented Jun 16, 2017

jsayol commented Jun 16, 2017 •

edited

Loading

googlebot commented Jun 16, 2017

jsayol commented Jun 16, 2017

jshcrowthe commented Jun 16, 2017

jshcrowthe commented Jun 16, 2017 •

edited

Loading

jsayol commented Jun 16, 2017

jsayol commented Jun 16, 2017 •

edited

Loading

jshcrowthe commented Jun 16, 2017 •

edited

Loading

jsayol commented Jun 16, 2017

jshcrowthe left a comment

jshcrowthe Jun 16, 2017 •

edited

Loading

jshcrowthe Jun 16, 2017

jsayol Jun 16, 2017

jshcrowthe Jun 16, 2017

jsayol Jun 16, 2017

jshcrowthe Jun 17, 2017 •

edited

Loading

jsayol Jun 17, 2017

jshcrowthe commented Jun 17, 2017

refactor(database): classes for typescript database implementation #55

refactor(database): classes for typescript database implementation #55

Conversation

jsayol commented Jun 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jshcrowthe Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jun 16, 2017

googlebot commented Jun 16, 2017

googlebot commented Jun 16, 2017

jsayol commented Jun 16, 2017 • edited Loading

googlebot commented Jun 16, 2017

jsayol commented Jun 16, 2017

jshcrowthe commented Jun 16, 2017

jshcrowthe commented Jun 16, 2017 • edited Loading

jsayol commented Jun 16, 2017

jsayol commented Jun 16, 2017 • edited Loading

jshcrowthe commented Jun 16, 2017 • edited Loading

jsayol commented Jun 16, 2017

jshcrowthe left a comment

Choose a reason for hiding this comment

jshcrowthe Jun 16, 2017 • edited Loading

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

jshcrowthe Jun 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jshcrowthe commented Jun 17, 2017

jsayol commented Jun 15, 2017 •

edited

Loading

jshcrowthe Jun 16, 2017 •

edited

Loading

jsayol commented Jun 16, 2017 •

edited

Loading

jshcrowthe commented Jun 16, 2017 •

edited

Loading

jsayol commented Jun 16, 2017 •

edited

Loading

jshcrowthe commented Jun 16, 2017 •

edited

Loading

jshcrowthe Jun 16, 2017 •

edited

Loading

jshcrowthe Jun 17, 2017 •

edited

Loading