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

Makes firestore a singleton and sets correct timestamp setting #48

Merged
merged 5 commits into from
May 10, 2018

Conversation

douweh
Copy link
Contributor

@douweh douweh commented May 10, 2018

I made the firestore instance a singleton on a newly created firestore service.
When asked for the instance I will also set the right settings getting rid of the deprecation warning in #43


export default Service.extend({
firebase: service(),
instance: computed('firebase', function() {
Copy link
Owner

Choose a reason for hiding this comment

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

Really appreciate for your time on taking a stab at this.

I'm not really liking the firestore.instance but I could see why you did it this way. I could probably refactor in the future should there be a cleaner solution that we can omit the .instance part. For now, this will do.

My only suggestion is maybe we can make this as not a computed property. We can assign the instance property in the init() hook. The settings will also be a standalone property so that other devs can override the service and provide their own settings if needed.

e.g.

export default Service.extend({
  ...

  settings: { timestampInSnapshots: true },

  init(...args) {
    this._super(...args);

    const firestore = this.get('firebase').firestore();

    firestore.settings(this.get('settings'));

    this.set('instance', firestore);
  }
});

Copy link
Contributor Author

@douweh douweh May 10, 2018

Choose a reason for hiding this comment

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

I agree on both items:

  • as a service needs to be an extension of Service, I could not see a way around this (the instance part) for now.
  • I changed the code so the instance gets set on init (and settings are overridable)

I pushed a new commit.

Changes I think could be added but should not prevent this PR from merging:
-> get settings from ENV
-> Update your mock-cloud-firestore so it implements a settings method (tests were failing because of that, so I added the if-guard)

douweh added 2 commits May 10, 2018 13:53
Signed-off-by: Douwe Homans <douwe@avocado.nl>
Signed-off-by: Douwe Homans <douwe@avocado.nl>
@mikkopaderes mikkopaderes merged commit 2bc7863 into mikkopaderes:master May 10, 2018
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.

2 participants