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

Merge torii fire #256

Merged
merged 2 commits into from Jun 29, 2015

Conversation

Projects
None yet
4 participants
@MattMSumner
Copy link
Contributor

MattMSumner commented May 22, 2015

This has two commits. The first extracts the Firbase instace into a service.

Why:

  • We'd like to use the firebase instance in multiple places, rather then
    importing the application adapter, it would be better to have a service
    that provides this.

This Commit:

  • Creates a firebased service that provides a global instance of
    Firebase.

The second Add a torii adapter for firebase.

Why:

  • It would be great to use firebase authentication more easily.

This Commit:

  • Merges the torii-fire addon into emberfire
  • This adds a torii adapter for firebase
@MattMSumner

This comment has been minimized.

Copy link
Contributor

MattMSumner commented May 22, 2015

I made a service for the firebase instance. I think this makes sense for if anyone who want's to grab that instance for other addons, however, the name I've picked is terrible so I'd love some suggestions.

Also I'm not sure if it makes sense to add documentation to the readme or to throw something into the wiki and link to that in the readme. Thoughts?

@MattMSumner

This comment has been minimized.

Copy link
Contributor

MattMSumner commented May 22, 2015

Also let me know if you want these split into two PRs.

@tstirrat

This comment has been minimized.

Copy link
Member

tstirrat commented May 22, 2015

I'm wondering if we can just use a resolver lookup for the firebase instance. The service feels like overkill.

this.container.lookup('firebase:main');
@tstirrat

This comment has been minimized.

Copy link
Member

tstirrat commented May 22, 2015

thanks for this btw!

@MattMSumner

This comment has been minimized.

Copy link
Contributor

MattMSumner commented May 26, 2015

So instead of using a service make an initializer to register firebase:main?

I think this type of dependency is perfectly suited for using a service. Using an initializer to register the firebase object feels like trying to get the same functionality of a service but throwing it into the container. It also means that anywhere we want to use it we'll need to use:

firebaseInstance: function() {
  return this.container.lookup('firebase:main').get('firebaseInstance');
}.property()

Which doesn't seem as elegant as using a service.

Thoughts?

@tstirrat

This comment has been minimized.

Copy link
Member

tstirrat commented May 27, 2015

I'd register the actual root reference as firebase:main rather than a containing object.. think store:main

It would be more like this:

// initializer
container.register('firebase:main', new Firebase(config.firebase), { instantiate: false });

// adapters/firebase.js 
// inside init()
this._ref = this.container.lookup('firebase:main');

// torii-providers/firebase.js
this.container.lookup('firebase:main').authWithOAuthPopup(...

then, if we really wanted (we don't really have a need atm) we could inject this into things:

container.inject('route', 'firebase', 'firebase:main');

and we could also modify the container to overload this reference when testing.. perhaps this will cleanup all the manual _ref set/restore code in the tests.

To me, a service implies that the firebased thing does something complex

Pre #249, users can access the firebase ref with this.container.lookup('firebase:main') but after it is merged they can get a records ref with record.ref()

@tstirrat tstirrat added the discussion label May 27, 2015

@MattMSumner MattMSumner force-pushed the MattMSumner:mms-merge-torii-fire branch from ad4d05c to 3bfaa5b Jun 28, 2015

@googlebot

This comment has been minimized.

Copy link

googlebot commented Jun 28, 2015

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case 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.
@MattMSumner

This comment has been minimized.

Copy link
Contributor

MattMSumner commented Jun 28, 2015

@tstirrat Sorry this has been sitting open for a while.

I'd like to talk again about using a service here instead of registering in an initializer. I believe ember-data has been moving towards using the store as a service rather then store:main. So when you need the store in a component you can simply say:

export default Ember.Component.extend({
  store: Ember.inject.service(),
  ...
})

This seems like the Ember 2.0 way of handling these sort dependencies that you may need to be globally available. I also prefer the idea of being explicit about when I need this dependency in my objects rather then, say, injecting it into all of the routes.

To me, a service describes something that needs to be globally available. The firebase instance seems to fix this description perfectly.

Thoughts?

@MattMSumner MattMSumner force-pushed the MattMSumner:mms-merge-torii-fire branch from 3bfaa5b to 387148a Jun 28, 2015

@MattMSumner

This comment has been minimized.

Copy link
Contributor

MattMSumner commented Jun 28, 2015

I've updated the service with some help from @rwjblue so it's usage is less offensive.

@MattMSumner MattMSumner force-pushed the MattMSumner:mms-merge-torii-fire branch from 387148a to 27df4cf Jun 28, 2015

@@ -0,0 +1,19 @@
import Ember from "ember";

This comment has been minimized.

@tstirrat

tstirrat Jun 29, 2015

Member

Please use single quotes

This comment has been minimized.

@MattMSumner

MattMSumner Jun 29, 2015

Contributor

👍

var self = this;

return new Ember.RSVP.Promise(function(resolve, reject) {
self.get("firebase").authWithOAuthPopup(options.authWith, function(error, authData) {

This comment has been minimized.

@tstirrat

tstirrat Jun 29, 2015

Member

single quotes again, and let's use arrow functions and remove the self var

@tstirrat

This comment has been minimized.

Copy link
Member

tstirrat commented Jun 29, 2015

A few small nitpicks, but otherwise looking good. I much prefer the new service syntax, thanks for updating it!

it('exists', function() {
const service = this.subject();
expect(service).to.be.ok;
});

This comment has been minimized.

@tstirrat

tstirrat Jun 29, 2015

Member

Maybe just assert service instanceof Firebase

This comment has been minimized.

@MattMSumner

MattMSumner Jun 29, 2015

Contributor

Good idea! Done.

@MattMSumner MattMSumner force-pushed the MattMSumner:mms-merge-torii-fire branch from 27df4cf to 89c950b Jun 29, 2015

MattMSumner added some commits May 15, 2015

Extracts the Firbase instace into a service
Why:

* We'd like to use the firebase instance in multiple places, rather then
importing the application adapter, it would be better to have a service
that provides this.

This Commit:

* Creates a firebased service that provides a global instance of
Firebase.
Add a torii adapter for firebase
Why:

* It would be great to use firebase authentication more easily.

This Commit:

* Merges the [torii-fire] addon into emberfire
* This adds a torii adapter for firebase

[torii-fire]: https://github.com/MattMSumner/torii-fire

@MattMSumner MattMSumner force-pushed the MattMSumner:mms-merge-torii-fire branch from 89c950b to f48e52a Jun 29, 2015

@MattMSumner

This comment has been minimized.

Copy link
Contributor

MattMSumner commented Jun 29, 2015

Comments should be addressed!

@tstirrat

This comment has been minimized.

Copy link
Member

tstirrat commented Jun 29, 2015

Just checking , have you signed the CLA?

@MattMSumner

This comment has been minimized.

Copy link
Contributor

MattMSumner commented Jun 29, 2015

@tstirrat just signed it.

tstirrat added a commit that referenced this pull request Jun 29, 2015

@tstirrat tstirrat merged commit d9e223a into firebase:master Jun 29, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@artburkart

This comment has been minimized.

Copy link

artburkart commented Jan 8, 2016

Is there any documentation for how to use this with custom authentication?

@tstirrat

This comment has been minimized.

Copy link
Member

tstirrat commented Jan 8, 2016

@artburkart use like this:

this.get('session').open('firebase', {
    provider: 'custom', 
    token: '<customToken>' // your custom generated token
  })
  .then(function(data) {
    console.log(data.currentUser);
  });

I've made a note to add custom auth info to the docs

@artburkart

This comment has been minimized.

Copy link

artburkart commented Jan 8, 2016

Hmmm, I did that and it didn't seem to work. I'll have to look into it more. Thanks!

@tstirrat

This comment has been minimized.

Copy link
Member

tstirrat commented Jan 9, 2016

It was added in #307

Make sure you're using EmberFire 1.6.0+ or at least one that contains this commit

@tstirrat

This comment has been minimized.

Copy link
Member

tstirrat commented Jan 9, 2016

If you find it is still not working in 1.6.0 please post your ember/emberfire/emberdata versions and any relevant console output in issue #307

@artburkart

This comment has been minimized.

Copy link

artburkart commented Jan 10, 2016

@tstirrat - thanks for the assistance. I figured it out! I implemented my custom authentication in elixir and my tokens were not of the proper format. I'm not sure where the authentication format is documented, but I figured it out after reading the nodejs firebase token generator source code. I had initially read the python one and it didn't make the format as clear, so I missed the d.

I found the token documentation here

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