Migration to Typescript #169

MichaelSolati · 2018-03-08T22:17:49Z

Description

This is essentially a conversion of the package to Typescript while also bumping Firebase support to v4.x.x.

Fixes #98

… timing

coveralls · 2018-03-11T07:23:13Z

Coverage increased (+1.005%) to 95.983% when pulling b5da5ee on MichaelSolati:typescript into 976556a on firebase:master.

asciimike · 2018-04-02T18:36:31Z

First off, thanks for the PR! This is a ton of work, and we're grateful for your contributions :)

I think the best way to handle this (ideally the fastest time to merge) is going to be as follows:

Split this in "half":
- One half that converts GeoFire as it exists to TypeScript
- One that adds Firestore support
PR the first into geofire-js:typescript
PR the second into geofire-js:firestore

I expect the first to be pretty straightforward, though I assume there will be questions around how this will be packaged, what versioning looks like, etc. Moving it somewhere other than master will let us get it into the repo and handle the packaging work without blocking the contribution.

I expect the second to be contentious for a few reasons:

It's not offered on Android and iOS
It's not clear if we want it in the same repo, or in the same package
The Firestore team would like to offer actual GeoQuery support in the future

MichaelSolati · 2018-04-05T16:11:24Z

Hey mcdonamp, thank you for the detailed reply. I'll take your advice and break this up so we start with just a PR for Typescript support. I'm assuming I can just reuse this PR for a TS PR, so I'll update this over the weekend.

MichaelSolati · 2018-04-10T15:16:36Z

@jshcrowthe I updated the code to best represent the suggestions made by @mcdonamp (Thank you guys for taking the time to look through this!)

jshcrowthe · 2018-04-10T19:42:08Z

Awesome! I will take a look. Thanks so much!

jshcrowthe

Hey @MichaelSolati!

Just wanted to make sure you know I'm in process of reviewing this (lots going on 😄). Starting at the top level (i.e. build/test/packaging stuff) had a couple of points of feedback (figured I'd give them in batches):

First and foremost, the committed package-lock.json caused my NPM install to fail w/ some proxy
issue.

npm ERR! code ENOTFOUND
npm ERR! errno ENOTFOUND
npm ERR! network request to https://registry.npmjs.org/ts-node/-/ts-node-5.0.1.tgz failed, reason: getaddrinfo ENOTFOUND registry.npmjs.org registry.npmjs.org:443
npm ERR! network This is a problem related to network connectivity.
npm ERR! network In most cases you are behind a proxy or have bad network settings.
npm ERR! network
npm ERR! network If you are behind a proxy, please make sure that the
npm ERR! network 'proxy' config is set properly.  See: 'npm help config'

Had to delete it and rerun npm install to get the dependencies set up.

Add a typings field to the package.json to ensure that users of the package get the benefits of the generated type definitions.
Add a prepare script to the package.json scripts section that will ensure this package gets built properly before release. (See https://docs.npmjs.com/misc/scripts on the prepare script)
Output an ESM module build in addition to UMD/CJS.
I'd suggest, instead of rolling your own build system using NPM scripts (e.g. rm -rf ./dist && tsc && npm run browserify && npm run uglify) that you use a build system like gulp, or a bundler like rollup. (Full disclosure we just migrated the firebase-js-sdk to a rollup based build system and I think you could re-use one of rollup.config.js files with very few edits here.)
We have a travis script in the package.json that isn't used anywhere. Do we need that?
In the travis.yml what was the rationale behind running npm run build after the tests have passed?
We don't actually run our tests in real browsers, is it possible to get something up and running w/ a browser test runner?
Does it make sense to set the importHelpers flag in our tsconfig.json and use tslib?
The lib property in the tsconfig.json is set to es2016, what was the intended browser support matrix here (I don't see us loading polyfills/shims anywhere)?

Figure I'll submit this here, more to come!

MichaelSolati · 2018-04-18T23:06:44Z

@jshcrowthe perhaps the most critical code review I've ever received, but I do appreciate it. Some of the choices I made aren't necessarily justified, so I spend the weekend polishing up based on your feedback. 😁

MichaelSolati · 2018-05-06T20:53:57Z

@jshcrowthe I'm 99% confident I made changes based on every point you raised. I also tested it with a personal project and everything seems to be working correctly with the new build, and it passes the tests still. I did try using the current version of geofire with the newly released Angular 6 (I created a project with the CLI), and there seems to be some issues... (These issues doesn't exist with this PR)

The only thing I couldn't get to (and am not sure if I can with time and other things on my plate), was looking into setting up a browser test runner. Hopefully that won't be an issue.

MichaelSolati · 2018-05-24T15:18:21Z

@jshcrowthe just wanted to bump this and see if there was any update to the status of this PR?

jshcrowthe · 2018-05-24T22:18:37Z

Hey @MichaelSolati appreciate the ping! I will go over this in the next few days and get back to you 😄

daveparks23 · 2018-06-08T02:50:10Z

Any update on this? Hoping to continue to use geoFire with Angular6 Thanks!

jshcrowthe

Hey @MichaelSolati great work here things are looking really good. I have added some specific things inline if you could address those that'd be great.

jshcrowthe · 2018-06-11T16:05:53Z

rollup.config.js

+const completeBuilds = [{
+    input: 'src/index.ts',
+    output: [{
+        file: pkg.browser,


We are shipping both a pkg.browser and a pkg.main that are both builds of the same file. Since this package doesn't require a Node/Browser specific entrypoint, let's remove the pkg.browser build from here and the package.json

Note: See https://github.com/jshcrowthe/howto-browser-modules for details

jshcrowthe · 2018-06-11T16:08:43Z

src/geoFire/index.ts

+import * as firebase from 'firebase';
+
+import { GeoQuery } from './geoQuery';
+import { decodeGeoFireObject, degreesToRadians, distance, encodeGeoFireObject, encodeGeohash, validateLocation, validateKey } from './geoFireUtils';


NIT: It seems as though we don't use degreesToRadians

jshcrowthe · 2018-06-11T16:18:36Z

src/geoFire/geoFireUtils.ts

+ * @param newQueryCriteria The criteria which specifies the query's center and/or radius.
+ * @param requireCenterAndRadius The criteria which center and radius required.
+ */
+export function validateCriteria(newQueryCriteria: any, requireCenterAndRadius: boolean = false): void {


We currently use the any type for newQueryCriteria here. It seems like we could probably provide an interface to help users input the proper object here.

jshcrowthe · 2018-06-11T16:24:45Z

src/geoFire/geoFireUtils.ts

+ * @param A Firestore snapshot.
+ * @returns The Firestore snapshot's id.
+ */
+export function geoFirestoreGetKey(snapshot: firebase.firestore.DocumentSnapshot): string {


This fxn is unused. Probably part of the 2nd half of your original PR?

jshcrowthe · 2018-06-11T16:26:51Z

src/geoFire/geoFireUtils.ts

+ * @returns The Firebase snapshot's key.
+ */
+export function geoFireGetKey(snapshot: firebase.database.DataSnapshot): string {
+  let key: string;


It looks like part of the function body was removed here (specifically the handling of snapshot.key fxn and a fallback snapshot.name.

What was the rationale?

I didn't realize that those were supported keys/attributes in older version of Firebase. I was basing it off of the typing from Firebase 4.13.1. Anyway I'll be adding it back in.

jshcrowthe · 2018-06-11T16:45:38Z

src/geoFire/geoQuery.ts

+  private _cancelled: boolean = false;
+  private _center: number[];
+  // A dictionary of geohash queries which currently have an active callbacks
+  private _currentGeohashesQueried: any = {};


We should probably provide an interface here as well. This should really be a map of string to the object here no?

jshcrowthe · 2018-06-11T16:48:49Z

src/geoFire/geoQuery.ts

+      });
+
+      // Add the geohash query to the current geohashes queried dictionary and save its state
+      this._currentGeohashesQueried[toQueryStr] = {


Lets define an interface for this object.

jshcrowthe · 2018-06-11T16:50:26Z

src/geoFire/geoQuery.ts

+      // Add the geohash query to the current geohashes queried dictionary and save its state
+      this._currentGeohashesQueried[toQueryStr] = {
+        active: true,
+        childAddedCallback: childAddedCallback,


NIT: Duplicating the key/val here is unnecessary. The following would work no?

this._currentGeohashesQueried[toQueryStr] = { active: true, childAddedCallback, childRemovedCallback, childChangedCallback, valueCallback };

jshcrowthe · 2018-06-11T16:52:22Z

src/geoFire/geoQuery.ts

+  private _currentGeohashesQueried: any = {};
+  // A dictionary of locations that a currently active in the queries
+  // Note that not all of these are currently within this query
+  private _locationsTracked: any = {};


Let's type this as well please.

jshcrowthe · 2018-06-11T16:53:25Z

src/geoFire/geoQuery.ts

+    isInQuery = (distanceFromCenter <= this._radius);
+
+    // Add this location to the locations queried dictionary even if it is not within this query
+    this._locationsTracked[key] = {


Similar to above, we should provide an interface for this type, and we can remove the duplication of identical key/val here.

MichaelSolati · 2018-06-12T06:44:41Z

@jshcrowthe I think and/or hope I hit all the points you mentioned. I was trying to keep it pretty much how the old JS version was, however I definitely agree with a lot of the changes you have suggested! (Especially the use of Maps).

The Maps did provide a small issue... I was hoping I could skip the turning the keys into an Array, and just iterate through the Map (there were a few places where that wouldn't have been appropriate, however for the most part it should have been fine). And while that did work and passed the tests, I did also get this weird error:

(node:34590) UnhandledPromiseRejectionWarning: AssertionError: expected [Error: Invalid Chai property: toBeFalsy] to be a function
    at failTestOnCaughtError (/geofire-js/test/common.ts:15:116)
    at process._tickCallback (internal/process/next_tick.js:68:7)
(node:34590) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:34590) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

I opted to turn the keys into an Array to avoid that weirdness...

jshcrowthe · 2018-06-12T19:03:00Z

I may have lead you astray slightly with the use of the word Map. I meant as in the annotation type

i.e.

const object: { [name: string]: SomeInterface } = { ... };

My apologies for the confusion. I'll go through the rest of the changes.

MichaelSolati · 2018-06-12T20:15:08Z

Ah... Well... Shucks! It seems to work fine anyway, and to be honest, I kind of prefer it. So no harm no fowl! 🐔 (Yes... Chicken puns...)

jshcrowthe

@MichaelSolati I think we are close here, thanks again for the hard work.

I left a couple comments inline. Additionally, using the Map collection and the Array.from syntax enforces ES2015 compatibility (unless we are going to polyfill for this lib, which I don't think we want?). Also, using the Map collection for string based keys is really a waste IMO.

Let's revert the Object -> Map refactor (but please include the Object version of the needed annotations).

jshcrowthe · 2018-06-20T18:57:48Z

src/geoFire/interfaces/geoQueryCallbacks.ts

@@ -0,0 +1,6 @@
+export interface GeoQueryCallbacks {
+  ready: any[];


In src/geoFire/interfaces/geoQueryState.ts we have this interface:

(a: firebase.database.DataSnapshot, b?: string) => any

If this is the callback type could we name it and instead of an any[] use that?

Ditto for the 3 props below

jshcrowthe · 2018-06-20T18:58:59Z

src/geoFire/interfaces/geoQueryState.ts

+
+export interface GeoQueryState {
+  active: boolean;
+  childAddedCallback: (a: firebase.database.DataSnapshot, b?: string) => any;


It seems like we could not repeat ourselves here with the callback signature. No?

Also, could we be more descriptive with the prop names than a and b?

While it may not be necessary, I'm not sure what a better/ideal solution would be. Effectively I can change it to whatever you thing is best. I'm just not sure what it would be off of the top of my head.

Lets just create a type and assign each of these to that?

export type GeoQueryStateCallback = (a: firebase.database.DataSnapshot, b?: string) => any; export interface GeoQueryState { childAddedCallback: GeoQueryStateCallback; ... }

As far as the property names go, I just looked at our .d.ts for the typescript package and we use the same variable names so I guess we are probably fine with that.

jshcrowthe

Looking really good, one last thing and then I think we are good to go!

jshcrowthe · 2018-06-26T21:36:37Z

src/geoFire/interfaces/geoQueryState.ts

+
+export interface GeoQueryState {
+  active: boolean;
+  childAddedCallback: (a: firebase.database.DataSnapshot, b?: string) => any;


Lets just create a type and assign each of these to that?

export type GeoQueryStateCallback = (a: firebase.database.DataSnapshot, b?: string) => any; export interface GeoQueryState { childAddedCallback: GeoQueryStateCallback; ... }

As far as the property names go, I just looked at our .d.ts for the typescript package and we use the same variable names so I guess we are probably fine with that.

MichaelSolati · 2018-06-28T16:51:08Z

@jshcrowthe done! Tests are running, and after your approval I think I'm going to get a beer.

jshcrowthe · 2018-06-29T21:32:00Z

Great work here @MichaelSolati!

jaufgang · 2018-08-15T23:19:05Z

Hey, @jshcrowthe, can we get a new @5.0.0 release of this package on npm with this PR merged in? People are resorting to installing the package from GitHub (see #173 (comment))

Thanks!

jaufgang · 2019-01-14T19:31:20Z

It's been over 6 months since this PR was merged but there has been no new release on NPM. This project seems to be basically abandoned. I get that no new dev work is going to happen here, but it would be great if at the very least someone could take the time to simply publish a new release on NPM based on this already-merged code.

@mikelehen, you seem to be active in reviewing/merging some of the recent PRs in the firebase-js-sdk project, so perhaps you could help out with this or at least know who to refer this to who might be able to help. Don't know who else to ask. Thanks!

asciimike · 2019-01-14T19:37:30Z

Josh left Google, so I assume that's why it's been dropped. I'll make sure it gets picked up and released.

MichaelSolati · 2019-01-14T20:30:15Z

Shucks... @mcdonamp I may ask for a little time to make some fixes/tweaks before you guys publish again. I've learned a lot while working on a diff firebase library and there are some things that should get addressed before a launch. I'll get on it and have a PR open by the end of the month.

Feiyang1 · 2019-01-15T02:41:43Z

Hey, let me know when you are done with the change. I will publish it to npm.

jaufgang · 2019-01-15T15:36:42Z

I'm pleased to see that my comment from yesterday may have kickstarted some momentum back into this project, but at least for me personally I would really prefer to see a new release published as soon as possible.

The current version of the code has been working well, but I keep getting these Cannot find module 'geofire' errors showing up on my build server or when deploying code depending on this library to Cloud Functions, and I am fairly confident that the problem is related to the fact that we need to install the package using a github repo link instead of a package link (i.e. npm i firebase/geofire-js instead of npm i geofire) this requires npm to run the prepare script on order to build the component on the client, and it just doesn't always seem to work properly and causes headaches, (see the discussion towards the bottom of #173).

@Feiyang1, can we get a 5.0.0 or even a 5.0.0.beta1 release for what has already been merged to master, and then when @MichaelSolati completes his new changes get another minor incremental release? I would be very grateful.

Feiyang1 · 2019-01-15T18:19:52Z

I can certainly publish a beta release and you can get it using geofire-js@next. Would it work for you?

Feiyang1 · 2019-01-15T18:30:43Z

Beta version published! https://www.npmjs.com/package/geofire?activeTab=dependents
You can get by running npm i geofire@next

MichaelSolati added 4 commits March 8, 2018 17:14

Migrate to Typescript

b0bec65

Fix additional mocha tests

41dff26

Fix events for GeoQuery class

6eb4280

Fix to kill mocha at end of test

e8e2ff5

MichaelSolati changed the title ~~Migrate to Typescript~~ Migrate to Typescript Fixes #98 Mar 9, 2018

MichaelSolati changed the title ~~Migrate to Typescript Fixes #98~~ Migrate to Typescript Mar 9, 2018

MichaelSolati added 8 commits March 9, 2018 01:43

feat(firestore): early implementation of geofirestore

51e76bd

test(firestore): implement test against realtime db to firestore

b73d800

test(geofirestore): check location against array instead of object

dd93bcd

test: fix tests for geofire callbacks and implement for geofirestore

8c007d8

fix(firestore): fix single location removal by set function

ffb377a

refactor: change vars to consts and use in instead of hasOwnProperty

4a0d312

fix(firestore): set data from snapshot to variable to pass into decode

e750c65

refactor: renamed folders and small tweaks

261445e

MichaelSolati changed the title ~~Migrate to Typescript~~ Firestore support (All In Typescript) Mar 11, 2018

MichaelSolati added 3 commits March 11, 2018 02:40

test: reintroduce coveralls support with tweaks

361fc5b

test: increase mocha timeout

aa7b084

test(firestore): modify "'key_exited' registrations can be cancelled"…

b5da5ee

… timing

asciimike requested a review from jshcrowthe April 2, 2018 16:31

MichaelSolati added 2 commits April 10, 2018 10:36

Merge branch 'master' into typescript

4c87223

chore: remove firestore support

ffd9a60

MichaelSolati changed the title ~~Firestore support (All In Typescript)~~ Migration to Typescript Apr 12, 2018

jshcrowthe suggested changes Apr 17, 2018

View reviewed changes

asciimike mentioned this pull request Apr 18, 2018

GeoFire is being abandoned? #158

Closed

MichaelSolati mentioned this pull request May 8, 2018

Angular 6 - Error while creating an instance of GeoFire #173

Closed

refactor(geofire): break out distance function

69ebd9b

jshcrowthe suggested changes Jun 11, 2018

View reviewed changes

MichaelSolati added 2 commits June 11, 2018 23:28

refactor: introduce new interfaces and use maps instead of dicts

a607d6e

fix: lock node version for tests and revert geoFireGetKey logic

154daa1

asciimike mentioned this pull request Jun 13, 2018

Allowing Firebase 4.x.x as a peer dependency #172

Closed

chore: update dependencies and support firebase 5.x.x

2859aa8

jshcrowthe suggested changes Jun 20, 2018

View reviewed changes

refactor(GeoQuery): revert to using dict instead of a Map

08eda8f

jshcrowthe suggested changes Jun 26, 2018

View reviewed changes

refactor(GeoQueryState): create interface for callbacks

7830059

jshcrowthe approved these changes Jun 29, 2018

View reviewed changes

jshcrowthe merged commit d5aecb8 into firebase:master Jun 29, 2018

		@@ -0,0 +1,6 @@
		export interface GeoQueryCallbacks {
		ready: any[];

Migration to Typescript #169

Migration to Typescript #169

Conversation

MichaelSolati commented Mar 8, 2018 • edited Loading

Description

coveralls commented Mar 11, 2018

asciimike commented Apr 2, 2018

MichaelSolati commented Apr 5, 2018

MichaelSolati commented Apr 10, 2018

jshcrowthe commented Apr 10, 2018

jshcrowthe left a comment • edited Loading

Choose a reason for hiding this comment

MichaelSolati commented Apr 18, 2018

MichaelSolati commented May 6, 2018 • edited Loading

MichaelSolati commented May 24, 2018

jshcrowthe commented May 24, 2018

daveparks23 commented Jun 8, 2018

jshcrowthe 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

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

MichaelSolati commented Jun 12, 2018

jshcrowthe commented Jun 12, 2018

MichaelSolati commented Jun 12, 2018

jshcrowthe 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

jshcrowthe Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

jshcrowthe left a comment

Choose a reason for hiding this comment

jshcrowthe Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

MichaelSolati commented Jun 28, 2018

jshcrowthe commented Jun 29, 2018

jaufgang commented Aug 15, 2018

jaufgang commented Jan 14, 2019

asciimike commented Jan 14, 2019

MichaelSolati commented Jan 14, 2019

Feiyang1 commented Jan 15, 2019

jaufgang commented Jan 15, 2019

Feiyang1 commented Jan 15, 2019

Feiyang1 commented Jan 15, 2019

MichaelSolati commented Mar 8, 2018 •

edited

Loading

jshcrowthe left a comment •

edited

Loading

MichaelSolati commented May 6, 2018 •

edited

Loading

jshcrowthe Jun 26, 2018 •

edited

Loading

jshcrowthe Jun 26, 2018 •

edited

Loading