Skip to content

Conversation

@rolandjitsu
Copy link

I was not able to test this as some of the npm deps could not be install, but I am sure that everything should be running properly.

Also, some of the npm packages are deprecated (gulp-karma), it may be a good idea to update the dev deps and the gulp tasks.

It might also be a good idea to start using ES6 and use a simple transpile to ES5 task for creating dist packages.

Implements #84.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@rolandjitsu
Copy link
Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@jwngr
Copy link

jwngr commented Feb 9, 2016

Thanks @rolandjitsu! This is awesome. Apologies on not having time to look at this just now. I plan to get to it in the near future though.

Also, some of the npm packages are deprecated (gulp-karma), it may be a good idea to update the dev deps and the gulp tasks.

I am aware of the out-of-date deps. The JS tooling community moves fast, which is both awesome and a bit daunting. I have a TODO to update all the deps and get the tests running once again. I'll do that before merging your change in.

It might also be a good idea to start using ES6 and use a simple transpile to ES5 task for creating dist packages.

Yeah this would be a good addition. This library was made before ES6 was the new hotness and the tooling wasn't as good back when I made this. I'll look into this time permitting.

@jwngr jwngr self-assigned this Feb 9, 2016
@rolandjitsu
Copy link
Author

It looks like JOB 2 always fails, but JOB 1 passes.

@rolandjitsu
Copy link
Author

@jwngr If you'd like, I'd be opened to do a little facelift to this lib (update the deps, use ES6, etc.). Unless you already have some ongoing work on it.

I plan on using this library and Firebase extensively on one of my projects, so I think it would be time well spent on developing on this lib.

As for merging the pull request, I do hope that you will have time within this week to do it, as I cannot use it until RSVP is gone from it.

@jwngr
Copy link

jwngr commented Feb 9, 2016

Feel free to take a crack at updating things. I'm more interested in updating the deps at present rather than using ES6 just yet, but if you can make it work in a backwards-compatible manner, go ahead. And yeah, I plan to look at before the end of this week!

@jwngr
Copy link

jwngr commented Feb 9, 2016

Note that I do actually have a branch which is still not working fully but has started to update dependencies: https://github.com/firebase/geofire-js/tree/jw-update-deps

@rolandjitsu
Copy link
Author

Great, then I will just try to update the deps and leave the to ES6 update for later. I will take a look at the branch and see if I can start from there.

And maybe at some point, when you think it would be best, I could try to do the update to ES6.

@jwngr
Copy link

jwngr commented Mar 5, 2016

Thanks again for the PR and sorry for the slow turnaround.

I'm closing this in favor of #93 which gets rid of the RSVP dependency entirely by just using the promise API built into Firebase 2.4.0 and higher. Once that gets merged in, I'll cut a 4.0.0 release for GeoFire and you can remove RSVP from your app!

@jwngr jwngr closed this Mar 5, 2016
@rolandjitsu
Copy link
Author

@jwngr sounds great 👍 Looking forward to it.

@jwngr
Copy link

jwngr commented Mar 6, 2016

GeoFire 4.0.0 has been released! Enjoy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants