refactor(database): Add typescript database implementation [Part 2/3] #62

jshcrowthe · 2017-06-16T19:28:17Z

Adds the new typescript implementation

googlebot · 2017-06-17T02:10:00Z

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.

jshcrowthe · 2017-06-19T16:54:12Z

@schmidt-sebastian It seems CLA bot doesn't like squash commits. This is fine to review/merge as you're able.

jshcrowthe · 2017-06-20T00:02:11Z

This PR presumes that #61 has been merged first

All integration tests pass at this point

…files

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

jshcrowthe · 2017-06-20T22:26:59Z

@schmidt-sebastian this has been rebased so it only includes the additions.

https://github.com/firebase/firebase-js-sdk/blob/fd0728138d88c454f8e38a78f35d831d6365070c/src/database/js-client/core/Repo.js#L86

schmidt-sebastian

Hi Josh, I think most of my comments are going to be similar to the ones I have in the first couple of files I reviewed. Can you take a quick look?

schmidt-sebastian · 2017-06-26T22:36:52Z

src/database/api/DataSnapshot.ts

+  }
+
+  get key() {
+    return this.getKey();


Does it make sense to make this the main codepath and call this from getKey()

schmidt-sebastian · 2017-06-26T22:49:03Z

src/database/api/DataSnapshot.ts

+  /**
+   * Returns whether the snapshot contains a non-null value.
+   *
+   * @return {boolean} Whether the snapshot contains a non-null value, or is empty.


As discussed over chat, we should figure out if we want to keep the type annotations in our comment or if it is sufficient to use the types from TypeScript.

Just chiming in since I was thinking about this the other day. Type annotations in the comments only make sense when using a tool like Closure compiler to do type checking, but it's not necessary anymore with TypeScript. Besides, I bet the types in the comments won't always be updated when the code changes so eventually it might do more harm than good.

I'm happy w/ defaulting to typescripts types. Just was trying to keep the delta small where possible.

That said, I think I'm going to merge #66 as I've been going through that and it addresses many of the type issues you've mentioned.

In addition I agree with @jsayol, that the divergence in types is probably a maintenance burden going forward. Let's consolidate after we've validated the code is good.

schmidt-sebastian · 2017-06-26T22:55:40Z

src/database/api/Database.ts

+ */
+export class Database {
+  /** @type {Repo} */
+  repo_;


I assume this can just use the typescript types (aka repo_: Repo)?

FYI there's another PR (#66) that builds on top of this one adding a whole bunch of types, const/let, private, etc.

I'll try and get @jsayol's PR merged as part of this.

EDIT: The PR mentioned has some tests that fail. I will work on consuming that afterwards.

schmidt-sebastian · 2017-06-26T22:56:43Z

src/database/api/Database.ts

+      fatal("Don't call new Database() directly - please use firebase.database().");
+    }
+
+    /** @type {Repo} */


This type information should now be unnecessary.

schmidt-sebastian · 2017-06-26T22:57:55Z

src/database/api/Database.ts

+    this.INTERNAL = new DatabaseInternals(this);
+  }
+
+  app: null


Add type information.

schmidt-sebastian · 2017-06-26T22:59:04Z

src/database/api/Database.ts

+   * @param {string=} opt_pathString
+   * @return {!Firebase} Firebase reference.
+   */
+  ref(opt_pathString): Reference {


Can we use 'pathString?: string' here?

schmidt-sebastian · 2017-06-26T22:59:53Z

src/database/api/Database.ts

+   */
+  refFromURL(url) {
+    /** @const {string} */
+    var apiName = 'database.refFromURL';


This should be 'const' instead of var.

Deferring to #66

schmidt-sebastian · 2017-06-26T23:01:01Z

src/database/api/Database.ts

+
+  /**
+   * @param {string} apiName
+   * @private


Please use 'private checkDeleted_()" instead.

schmidt-sebastian · 2017-06-26T23:01:58Z

src/database/api/Database.ts

+};
+
+// Note: This is an un-minfied property of the Database only.
+Object.defineProperty(Database.prototype, 'app', {


Here and below: Can this be a getter in the main class?

jsayol · 2017-06-27T22:09:28Z

src/database/api/Database.ts

+    return this.repo_.app;
+  }
+
+  get database(): Database {


The database getter actually belongs in Repo

Great catch! Thank you!

schmidt-sebastian · 2017-06-28T22:28:40Z

src/database/api/Query.ts

+   * @param {(function(!DataSnapshot, ?string=))=} callback
+   * @param {Object=} context
+   */
+  off(eventType?: string, callback?: (a: DataSnapshot, b?: string | null) => any, context?: Object) {


Can we introduce a type declaration for this?

schmidt-sebastian · 2017-06-28T22:31:29Z

src/database/api/Query.ts

+    }
+
+    // Calling with no params tells us to start at the beginning.
+    if (value == null) {


the original code uses undefined

There's a parameter initializer on value (= null) so by the time it reaches this check it will never be undefined.

schmidt-sebastian · 2017-06-28T22:42:28Z

src/database/api/TransactionResult.ts

+   * @param {boolean} committed
+   * @param {fb.api.DataSnapshot} snapshot
+   */
+  constructor(committed, snapshot) {


Can you let TS do the member assignment here directly?

schmidt-sebastian · 2017-06-28T22:43:27Z

src/database/api/onDisconnect.ts

+   * @param {function(?Error)=} opt_onComplete
+   * @return {!firebase.Promise}
+   */
+  cancel(opt_onComplete) {


Here and everywhere - type should be: onComplete?

Done (I also did it for all the declarations below)

schmidt-sebastian · 2017-06-28T23:03:34Z

As discussed over Hangouts, this is good to merge. I went through the first 25+ files in detail and most of the issues I encountered are getting addressed in subsequent PRs (and all of them are just to make the codebase more TS-like). The tests are also passing at this point and we should merge this into the TS branch.

@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

jshcrowthe added the api: database label Jun 16, 2017

jshcrowthe assigned schmidt-sebastian Jun 16, 2017

jshcrowthe requested a review from schmidt-sebastian June 16, 2017 19:28

google-oss-bot added the needs-triage label Jun 16, 2017

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

jshcrowthe removed the needs-triage label Jun 16, 2017

jshcrowthe mentioned this pull request Jun 16, 2017

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

Merged

jshcrowthe mentioned this pull request Jun 20, 2017

WIP: refactor(database): Add migrated test harness [Part 3/3] #71

Merged

jshcrowthe and others added 5 commits June 20, 2017 15:25

refactor(database): add typescript implementation

4a0df56

refactor(database): update build process to include database.ts

dcf2f5c

All integration tests pass at this point

refactor(*): refactor environment builds to be based on separate .ts …

7e609d6

…files

WIP: patch database code in nodeJS

e72bfbe

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

2f6a406

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

jshcrowthe force-pushed the ts-database/add-new-impl branch from 7efa1b0 to 2f6a406 Compare June 20, 2017 22:26

jshcrowthe mentioned this pull request Jun 20, 2017

refactor(database): Implement database in Typescript #72

Merged

jsayol mentioned this pull request Jun 22, 2017

fix(database): Add missing "typeof" #74

Merged

fix(database): Add missing "typeof" (#74)

cf357d0

https://github.com/firebase/firebase-js-sdk/blob/fd0728138d88c454f8e38a78f35d831d6365070c/src/database/js-client/core/Repo.js#L86

schmidt-sebastian reviewed Jun 26, 2017

View reviewed changes

jsayol mentioned this pull request Jun 27, 2017

refactor(database): Add types to several classes and utility methods #66

Merged

jshcrowthe added 2 commits June 27, 2017 14:21

WIP: fixes from @schmidt-sebastian's review

61b8a48

WIP: fix: TS Build error

71e4440

jsayol reviewed Jun 27, 2017

View reviewed changes

fix(database): fix issue with missing repo method

25aeeed

schmidt-sebastian reviewed Jun 28, 2017

View reviewed changes

WIP: review adjustments #1

61c8d25

jshcrowthe force-pushed the ts-database/add-new-impl branch from f108014 to 61c8d25 Compare June 28, 2017 22:43

schmidt-sebastian reviewed Jun 28, 2017

View reviewed changes

WIP: review comments #2

a01ff31

jshcrowthe merged commit bb80d31 into ts-database/final Jun 28, 2017

jshcrowthe deleted the ts-database/add-new-impl branch June 28, 2017 23:03

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): Add typescript database implementation [Part 2/3] #62

refactor(database): Add typescript database implementation [Part 2/3] #62

jshcrowthe commented Jun 16, 2017

googlebot commented Jun 17, 2017

jshcrowthe commented Jun 19, 2017

jshcrowthe commented Jun 20, 2017

jshcrowthe commented Jun 20, 2017

schmidt-sebastian left a comment

schmidt-sebastian Jun 26, 2017

schmidt-sebastian Jun 26, 2017

jsayol Jun 27, 2017

jshcrowthe Jun 27, 2017

jshcrowthe Jun 27, 2017

schmidt-sebastian Jun 26, 2017

jsayol Jun 27, 2017

jshcrowthe Jun 27, 2017 •

edited

Loading

schmidt-sebastian Jun 26, 2017

schmidt-sebastian Jun 26, 2017

schmidt-sebastian Jun 26, 2017

jshcrowthe Jun 27, 2017

schmidt-sebastian Jun 26, 2017

jshcrowthe Jun 27, 2017

schmidt-sebastian Jun 26, 2017

jshcrowthe Jun 27, 2017

schmidt-sebastian Jun 26, 2017

jsayol Jun 27, 2017

jshcrowthe Jun 27, 2017

schmidt-sebastian Jun 28, 2017

jshcrowthe Jun 28, 2017

schmidt-sebastian Jun 28, 2017

jsayol Jun 28, 2017

jshcrowthe Jun 28, 2017

schmidt-sebastian Jun 28, 2017

jshcrowthe Jun 28, 2017

schmidt-sebastian Jun 28, 2017 •

edited

Loading

jshcrowthe Jun 28, 2017

schmidt-sebastian commented Jun 28, 2017

refactor(database): Add typescript database implementation [Part 2/3] #62

refactor(database): Add typescript database implementation [Part 2/3] #62

Conversation

jshcrowthe commented Jun 16, 2017

googlebot commented Jun 17, 2017

jshcrowthe commented Jun 19, 2017

jshcrowthe commented Jun 20, 2017

jshcrowthe commented Jun 20, 2017

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

jshcrowthe Jun 27, 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

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 Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian commented Jun 28, 2017

jshcrowthe Jun 27, 2017 •

edited

Loading

schmidt-sebastian Jun 28, 2017 •

edited

Loading