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

Support for firestore #62

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

rickerbh
Copy link

@rickerbh rickerbh commented Jun 1, 2018

I've added Google's Firebase Firestore as a data source/destination in view model. If there is anything else that needs to be implemented or supported to get this database into the package just let me know.

@adrai
Copy link
Contributor

adrai commented Jun 1, 2018

wow cool.
Is there a way to have tests?

just another idea:
For simple queries, would it be possible to map mongodb like queries?
I.e.
this: aRepo.find({aProp: 'aValue'},
would be mapped to this: aRepo.find(['aProp', '==', 'aValue'],
?

@nanov
Copy link
Contributor

nanov commented Jun 1, 2018

Looks great indeed!
In addition to tests ( let us know if you need any help setting those up ), could you please adjust the documentation to use ES5 syntax, for it to be in tact with the rest?

@rickerbh
Copy link
Author

rickerbh commented Jun 1, 2018

@adrai I've now included support for both mongodb like queries, as well as the firestore array syntax.
@nanov I've updated the documentation as well, and (I think) got all of the ES6 out of there.

Regarding tests - what's the best way to set these up? Firestore is hosted by google, so credentials will be needed to get tests to hit the real implementation.

@rickerbh
Copy link
Author

rickerbh commented Jun 1, 2018

Actually, I think I've got this. I'll change the .travis.yml file to write out an environment variable with the credentials in it to a file, and use that for authentication with Google. I'll get those changes back here soon.

@rickerbh
Copy link
Author

rickerbh commented Jun 4, 2018

I've got all the tests passing now (bar the ones I've excluded with nested array support). I needed to add support for more functionality and concurrency checks - which is a good thing. I've also updated README.md with instructions on how to setup travis to work with the data store. Can you let me know if you feel anything is missing?

@adrai
Copy link
Contributor

adrai commented Jun 4, 2018

looks good...
is there a reason why the tests are excluded?

@rickerbh
Copy link
Author

rickerbh commented Jun 4, 2018

Yes, it's because it would fail the tests if you merge to master, because the environment variable in your travis for the authentication with google won't be set up. I left the config for it commented out so you could add the authentication whenever you wanted, and wouldn't break existing tests.

@nanov
Copy link
Contributor

nanov commented Jun 4, 2018

Hi, i have no experience with firestore, and correct me if I'm wrong, but it seems like this is aimed at testing.

@rickerbh
Copy link
Author

rickerbh commented Jun 4, 2018

@nanov That's for testing cloud functions only - not the datastore itself. It simulates triggering the cloud functions once data in inserted/updated/deleted.

I might be able to use https://www.npmjs.com/package/mock-cloud-firestore to mock out the datastore if you like.

@nanov
Copy link
Contributor

nanov commented Jun 4, 2018

Hmm, as said, i have zero experience with Google Firebase Firestore so I cannot really tell, what other options there are to achieve automated testing? Maybe setting up a dedicated test account by google for this library?

EDIT: Are those things usually done with this mock library?

@rickerbh
Copy link
Author

rickerbh commented Jun 4, 2018

I had a look at that mocking library and it doesn't appear to support the features required for the concurrency checks. I think the only way to get automated testing is via setting up a dedicated account. But the owner of the project should probably own the account. I'm happy to set it up, and then transfer ownership to someone else.

@adrai
Copy link
Contributor

adrai commented Jun 4, 2018

Ok... then you are the owner of the project (at least for this db implementation) ;-)

@rickerbh
Copy link
Author

Sorry for the long gap in communications - what's the best way to get the Firebase credentials to you? They shouldn't be exposed publicly.

@adrai
Copy link
Contributor

adrai commented Jul 21, 2018

perhaps if i give you access to the repo, you can encrypt the credentials directly for travis?
So they are never exposed.
Never tried this...

@rickerbh
Copy link
Author

Hi, yes I can. That's no problem. Just need to paste in 1 variable in the config in travis, then uncomment the tests and it should just work. Let me know when access is granted, and I'll get it set up.

@adrai
Copy link
Contributor

adrai commented Jul 24, 2018

invited ;-)

@rickerbh
Copy link
Author

Thanks for that. I've just had a look at travis and I can't see the project in my repositories list yet. Hopefully it'll appear after a sync or something.

_ = require('lodash'),
async = require('async'),
ConcurrencyError = require('../concurrencyError'),
gcFirestore = require('@google-cloud/firestore'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use Repository.use('@google-cloud/firestore') instead of require('@google-cloud/firestore') ?

@rickerbh rickerbh force-pushed the firestore branch 5 times, most recently from f2e1e9b to ec529d1 Compare January 22, 2019 00:39
- Firebase only supports 500 changes as part of a batch, so this
  function needs to loop through larger collections.
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