Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow data to be stored on GeoFire Index #45

Closed
wants to merge 8 commits into from

Conversation

mikepugh
Copy link
Contributor

@mikepugh mikepugh commented Sep 5, 2014

I've updated the code to allow optionally adding a data element onto the GeoFire index. One new method was added "getWithData" to keep the existing "get" method backwards compatible. The "key_entered" and "key_moved" callbacks have been enhanced with a new data parameter which will be populated if data was stored on the index, otherwise it is null.

I also added .catch() statements to all of the unit tests dealing with promises. The common error handler will fail the Jasmine test and show the stack trace. I found this helpful when originally my tests were failing but the only indication of failure was the Jasmine ASYNC timeouts, and no obvious reasons for why the test failed. It turned out I was attempting to set a Firebase location to 'undefined', so the Firebase library itself threw an error, which was swallowed by the RSVP promise.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) when pulling 63eea02 on mikepugh:master into 7cccee7 on firebase:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) when pulling 08a8a23 on mikepugh:master into 7cccee7 on firebase:master.

@jwngr
Copy link

jwngr commented Sep 15, 2014

Thanks for sending this my way @mikepugh! I'm surprised at how few lines of code it took to actually get this working. I very much appreciate the addition of tests and code comments/documentation. You get a thumbs up from me. However, for now I'm going to close this PR since we don't want to introduce this into the library right now. We will continue to have discussions around this and it may make a triumphant return at some point, but that time is not now.

I took some of the things I liked from this PR (the catch() for all tests, comment cleanup) and put it in a separate PR #48). I think those are good additions to the library!

@jwngr jwngr closed this Sep 15, 2014
@jwngr
Copy link

jwngr commented Sep 15, 2014

Also, FYI that we intentionally don't include the distribution files in this repo anymore. We don't want people pulling the files from there as they are not guaranteed to correspond to an actual release. We want people to get them from bower, npm, the Firebase CDN, or the GitHub releases page for this repo.

@mikepugh
Copy link
Contributor Author

Sounds good @jwngr . I'm going to maintain the PR in my forked repo in case anyone needs it. Also, how do you get bower to pick up the distribution files w/o including them in the repo? When you do a release, do you force add the distro files - tag & publish - and then delete the distro files?

@jwngr
Copy link

jwngr commented Sep 15, 2014

Cool, I'll point people to your fork if anyone else asks for this feature. And if enough people ask, we will probably add it ourselves.

And yup, what you suggested is exactly what we do. With all the open source projects we maintain, it got a bit cumbersome to have a different deploy process for each one. So I put together a common deploy process through Jenkins and generalized it across a lot of our repos. The deploy process will create a new commit (e.g. this is the one for 3.0.2) which has the checked-in dist files and the correct version numbers and then will tag that as the released version. That way Bower will have the files it needs. Then, the deploy process follows up with another commit to check-out the dist files (e.g. here is the one for 3.0.2).

It's a bit hacky, but our previous deploy processes were very manual and error-prone. Now, it just requires us to type in a semver and click a button. Then wait two minutes for all the magic to happen!

@morrislaptop
Copy link

morrislaptop commented May 18, 2018

@mikepugh FYI I've migrated your PR over to https://github.com/MichaelSolati/geofirestore. Except I created a setWithData method as well for more BC.

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.

None yet

4 participants