refactor(database): Add types to several classes and utility methods by jsayol · Pull Request #66 · firebase/firebase-js-sdk

jsayol · 2017-06-17T15:02:45Z

Refactoring of several classes and utility methods to add proper types and annotations

jsayol · 2017-06-17T15:06:08Z

This condition would never be true since it's comparing a number with a string. It was probably meant to say
if (c + segsize > str.length) {

This has probably gone undetected for a while since it didn't cause any problems anyway. According to MDN for substring: If either argument is greater than stringName.length, it is treated as if it were stringName.length.

We might as well remove the condition altogether?

jsayol · 2017-06-17T15:28:44Z

There was something weird here between the module-wide logger variable and the parameter passed to the enableLogging() function.

Edit: I just checked the original JS code and the module-wide variable was fb.core.util.logger, so that explains it. Make sure to review these changes with special care anyway.

Great catch on this. This is, as you pointed out, a bug from migration.

jsayol · 2017-06-17T15:36:52Z

startEndSet_ doesn't exists in this class. I'm 99.9% sure this was meant to say endNameSet_, but double-check just in case.

Edit: the same mistake is present in the current JS code. It's probably a good idea to check whether fixing this might somehow introduce a breaking change, in case other parts of the code are unknowingly relying on the current behavior.

Edit 2: nah, it's safe to make this change. The way it currently works, calling QueryParams#endAt() would fail to set endNameSet_ to false when called without a key name, but it was already initialized as false anyway. Subsequent calls to endAt() are not allowed by Query, so it never changes after that.

jshcrowthe · 2017-06-19T18:28:33Z

I'm going to try and dig into this in the next couple of days. I want to get the test harness in a place where it is available first. Initial look is good though!

jsayol · 2017-06-20T09:23:47Z

Sure @jshcrowthe, no hurry!

I was taking a look at the new tests and I realized it will be necessary to make a few changes to some of them if this PR is merged, specially when using classes like SortedMap, LLRBNode, ImmutableTree, etc, since I added generic types.

Let me know when that PR is merged and I'll update this one with any necessary changes, ok?

jsayol · 2017-06-22T11:07:45Z

I removed this clearTimeout since this.onClose is a function. Probably left over from a previous change.

jsayol · 2017-06-22T11:13:30Z

I had to force-push some changes to the first commit. If you already pulled it earlier then make sure to pull the latest version.

jshcrowthe

I have gone through this and it all looks pretty good. There are some stylistic differences here but I plan on hitting the whole project with the prettier hammer here after this PR is merged so I'm going to ignore that for now.

jshcrowthe · 2017-06-27T17:56:54Z

I noticed there is a couple places in this file where we do the <T> casting. We currently aren't using TSX but if we wanted to we would probably need to migrate these to do as casting.

This is true for all instances of this

jshcrowthe · 2017-06-27T18:54:30Z

The test harness fails on this with a slew of errors. Please point this to ts-database/final and as we are able to get the tests PR in we can fix these issues.

jsayol · 2017-06-27T18:57:28Z

Yeah, that's probably because of the generic types that I mentioned. I'll look into it.

jsayol · 2017-06-27T19:28:40Z

If I point this PR to the ts-database/final branch then it will also bring all the commits from PR #62, since this one is built on top of those changes. It might be a bit of a mess.

Should I do it anyway and then rebase later when that PR is merged?

jsayol · 2017-06-28T21:51:00Z

Just for reference, this is the result of my build and these numbers are after minification:

File	Parsed Size	Gzip Size
firebase-app.js	17.94 KB	5.97 KB
firebase-auth.js	135.47 KB	42.17 KB
firebase-database.js	1.29 MB	370.1 KB
firebase-messaging.js	24.93 KB	6.59 KB
firebase-storage.js	54.02 KB	13.4 KB
firebase.js	1.51 MB	436.35 KB

So clearly something's amiss. Besides that, I just merged my changes into the ts-database/migrate-test-harness branch locally and I can't get the tests to run because of a missing file:

[23:43:01] 'runAllKarmaTests' errored after 9.86 s
[23:43:01] Error: Unable to resolve module [../../config/project.json] from [/home/josep/projects/firebase-js-sdk/tests/database/helpers/util.js]

I think you're right, I probably messed up a merge at some point. I'll see if I can sort this out tomorrow, thanks for the patience :)

jsayol · 2017-06-29T00:56:45Z

Ok, I've solved all the merge issues. Sorry for all the noise about this. There's a handful of tests failing but I'll look into that tomorrow.

jsayol · 2017-06-29T08:21:05Z

Only 2 tests are failing on my end:

Connection -> disconnect old session on new connection: It's marked as flaky in the comments, so I assume it's known to fail some times?
Transaction Tests -> Transaction aborts after 25 retries.: It's actually working as expected (it does abort after 25 retries) but it's erroring due to the 2000ms timeout, while in my test it takes about 3200ms on average to complete.

jshcrowthe · 2017-06-29T17:35:56Z

@jsayol perfect, as part of the review for #71 we are adding a global timeout of 5000ms to the tests. So that should hopefully solve your issue. Going forward we need to find a way to fix the coupling to the prod database for these tests. But that can be a separate issue. That would help solve some of the volatility in these tests.

In addition, it seems I forgot to skip the test in the Node test setup. I have skipped the test and pushed it to ts-database/migrate-test-harness.

jshcrowthe · 2017-07-06T19:57:17Z

Hey @jsayol we have gotten all of the PR's merged, if you'll please rebase we can get this merged.

jshcrowthe · 2017-07-06T22:15:00Z

I have pointed this PR to the master branch as we have merged #72

jsayol · 2017-07-06T22:38:38Z

if you'll please rebase we can get this merged

Sure, I'll try to get around to it this weekend.

* refactor(database): Several classes and utility methods * WIP: fix tests with recent type changes * refactor(database): Type casting format for TSX support * refactor(database): Add 'noImplicitAny' support Adds support for the 'noImplicitAny' Typescript compiler option to the database implementation.

jsayol · 2017-07-08T10:14:23Z

@jshcrowthe done.

jshcrowthe

Nice work here @jsayol! I want to get #94 and #90 merged before the release next week. That should get us to a nice working state for the database code.

jshcrowthe · 2017-07-08T20:51:17Z

Great catch on this. This is, as you pointed out, a bug from migration.

jshcrowthe · 2017-07-08T20:56:28Z

I have run tests locally and they all pass, Travis-CI seems to have hung, so I am going to merge this.

jsayol · 2017-07-08T23:19:35Z

I want to get #94 and #90 merged before the release next week.

Nice! I'll make sure to get #94 ready by then.

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

jsayol commented Jun 17, 2017

View reviewed changes

jsayol mentioned this pull request Jun 18, 2017

refactor(database): ServerActions abstract class and implementations #65

Closed

jsayol force-pushed the ts-database-changes branch from 60a103f to f86c8e9 Compare June 18, 2017 21:54

jshcrowthe self-requested a review June 19, 2017 18:25

jshcrowthe self-assigned this Jun 19, 2017

jshcrowthe added api: database and removed needs-triage labels Jun 19, 2017

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

jsayol force-pushed the ts-database-changes branch 6 times, most recently from d0c40aa to 7eb6213 Compare June 22, 2017 11:05

jsayol commented Jun 22, 2017

View reviewed changes

jsayol force-pushed the ts-database-changes branch from 7eb6213 to e975c0c Compare June 22, 2017 20:46

jsayol mentioned this pull request Jun 27, 2017

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

Merged

jshcrowthe reviewed Jun 27, 2017

View reviewed changes

jsayol force-pushed the ts-database-changes branch 3 times, most recently from b128848 to 3b95509 Compare June 27, 2017 22:06

jsayol force-pushed the ts-database-changes branch 3 times, most recently from 8406db5 to d9fea62 Compare June 29, 2017 00:54

jsayol force-pushed the ts-database-changes branch from d9fea62 to 322c54a Compare June 29, 2017 08:11

jsayol mentioned this pull request Jun 30, 2017

FR: Offline first support for PWAs (RTDB) #17

Open

jshcrowthe added the Status: Pending Response label Jul 6, 2017

jshcrowthe changed the base branch from ts-database/final to master July 6, 2017 22:14

jsayol requested review from mikelehen and schmidt-sebastian as code owners July 8, 2017 09:23

jsayol force-pushed the ts-database-changes branch 2 times, most recently from a92520a to 4d30da1 Compare July 8, 2017 09:38

jsayol force-pushed the ts-database-changes branch from 4d30da1 to eca9bdd Compare July 8, 2017 09:50

jsayol changed the title ~~refactor(database): several classes and utility methods~~ refactor(database): Add types to several classes and utility methods Jul 8, 2017

jsayol mentioned this pull request Jul 8, 2017

style(*): Automate Code Formatting #90

Merged

jshcrowthe approved these changes Jul 8, 2017

View reviewed changes

jshcrowthe merged commit 00f9c37 into firebase:master Jul 8, 2017

jshcrowthe removed the Status: Pending Response label Jul 8, 2017

jsayol deleted the ts-database-changes branch July 13, 2017 07:20

firebase locked and limited conversation to collaborators Oct 27, 2019

Uh oh!

Conversation

jsayol commented Jun 17, 2017

Uh oh!

jsayol Jun 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsayol Jun 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jshcrowthe Jul 8, 2017

Choose a reason for hiding this comment

Uh oh!

jsayol Jun 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jshcrowthe commented Jun 19, 2017

Uh oh!

jsayol commented Jun 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsayol Jun 22, 2017

Choose a reason for hiding this comment

Uh oh!

jsayol commented Jun 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jshcrowthe left a comment

Choose a reason for hiding this comment

Uh oh!

jshcrowthe Jun 27, 2017

Choose a reason for hiding this comment

Uh oh!

jsayol Jun 27, 2017

Choose a reason for hiding this comment

Uh oh!

jshcrowthe commented Jun 27, 2017

Uh oh!

jsayol commented Jun 27, 2017

Uh oh!

jsayol commented Jun 27, 2017

Uh oh!

jsayol commented Jun 28, 2017

Uh oh!

jsayol commented Jun 29, 2017

Uh oh!

jsayol commented Jun 29, 2017

Uh oh!

jshcrowthe commented Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jshcrowthe commented Jul 6, 2017

Uh oh!

jshcrowthe commented Jul 6, 2017

Uh oh!

jsayol commented Jul 6, 2017

Uh oh!

jsayol commented Jul 8, 2017

Uh oh!

jshcrowthe left a comment

Choose a reason for hiding this comment

Uh oh!

jshcrowthe Jul 8, 2017

Choose a reason for hiding this comment

Uh oh!

jshcrowthe commented Jul 8, 2017

Uh oh!

jsayol commented Jul 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jsayol Jun 17, 2017 •

edited

Loading

jsayol Jun 17, 2017 •

edited

Loading

jsayol Jun 17, 2017 •

edited

Loading

jsayol commented Jun 20, 2017 •

edited

Loading

jsayol commented Jun 22, 2017 •

edited

Loading

jshcrowthe commented Jun 29, 2017 •

edited

Loading