mainmatter / ember-simple-auth

A library for implementing authentication/authorization in Ember.js applications.
https://ember-simple-auth.com
MIT License
1.92k stars 603 forks source link

Deprecation warning in latest ember, since access to instances in initializers is deprecated #517

Closed koemeet closed 9 years ago

koemeet commented 9 years ago

Hi,

In the latest ember version, you cannot access instances of a container in the intializer anymore, see: http://emberjs.com/deprecations/v1.x/#toc_deprecate-access-to-instances-in-initializers. Currently, this library triggers a lot of deprecation errors in latest ember version.

Any plans on updating simple-auth to support later ember versions?

jasich commented 9 years ago

:+1:

marcoow commented 9 years ago

PR would be great. Of course that would have to be backwards compatible and also work with Ember versions that don't have instance initializers.

EmergentBehavior commented 9 years ago

:+1:

Keeo commented 9 years ago

:+1:

mattma commented 9 years ago

+1

rbwsam commented 9 years ago

:+1:

jozefvaclavik commented 9 years ago

:+1:

johnhamelink commented 9 years ago

Yes please! :+1:

jzgit commented 9 years ago

:+1:

vmalloc commented 9 years ago

:+1:

hooddanielc commented 9 years ago

:+1:

kaiwah commented 9 years ago

:thumbsup:

GeeWizWow commented 9 years ago

:+1:

felixkiss commented 9 years ago

:+1:

akisvolanis commented 9 years ago

+1

liclac commented 9 years ago

:+1:

koemeet commented 9 years ago

:+1:

quaertym commented 9 years ago

We need more :+1:

koemeet commented 9 years ago

@quaertym Yes indeed :+1:

engwan commented 9 years ago

:+1:

akatov commented 9 years ago

:+1:

mikejerome commented 9 years ago

:+1:

Awem commented 9 years ago

I guess the need to be backwards compatible makes this overly complicated. I've just refactored my Simple-Auth-Custom-Session to use an Instance Initializer. It works nice, but a backwards compatible version would be a horrible code mess.

marcoow commented 9 years ago

It needs to be backwards compatible. We cannot just force everyone to update to the latest ember.

chrisvdp commented 9 years ago

But you can tell people on older Ember versions to use older simple-auth versions. example

marcoow commented 9 years ago

That would be a pain though as either people on old Ember versions wouldn't get bug fixes anymore or we would have to maintain 2 versions of Ember Simple Auth. I'm sure we can figure a backwards compatible solution out.

ofridagan commented 9 years ago

@AW-UC could you please share what you did?

miguelcobain commented 9 years ago

:+1:

Awem commented 9 years ago

As requested, I share my refactored code. Before, I used a custom session like this:

// app/sessions/custom.js
import Ember from 'ember';
import DS from 'ember-data';
import Session from 'simple-auth/session';

export default Session.extend({
  currentUser: Ember.computed('secure.user_id', function() {
    var userId = this.get('secure.user_id');
    if (!Ember.isEmpty(userId)) {
      return DS.PromiseObject.create({
        promise: this.container.lookup('store:main').find('user', userId)
      });
    }
  })
});

The container.lookup() is deprecated here. If one wants to stick to registering a custom session, this would be a possible solution:

// app/sessions/custom.js
import Session from 'simple-auth/session';
export default Session.extend({
  property: 'value'
//...
});
// app/instance-initializers/custom-session.js
//...
function initialize(application) {
  var session = application.container.lookup('session:custom');
  var store = application.container.lookup('store:main');

  session.currentUser = Ember.computed('secure.user_id', function() {
    //...
  })
}
//...

IMHO, you only want to do this, if for some reason you still need to define properties during Session.extend() or you want to refactor it in a backwards compatible way. In the later case, you would do stuff in Session.extend() under the condition that Ember.FEATURES.isEnabled('ember-application-instance-initializers') === false. In most cases, I think it is cleaner to not register a custom session at all, but do a custom instance initialization of the SimpleAuthSession. So in your environment.js you remove e.g. session: 'session:custom' from ENV['simple-auth'] and delete e.g. app/sessions/custom.js. Then you just do:

// app/instance-initializers/custom-session.js
import Ember from 'ember';
import DS from 'ember-data';

function initialize(application) {
  var session = application.container.lookup('simple-auth-session:main');
  var store = application.container.lookup('store:main');

  session.reopen({
    currentUser: Ember.computed('secure.user_id', function() {
      var userId = this.get('secure.user_id');
      if (!Ember.isEmpty(userId)) {
        return DS.PromiseObject.create({
          promise: store.find('user', userId)
        });
      }
    })
  });
}

export default {
  initialize: initialize
};

This latest example, is what I do in my app and it works fine. But suggestions for improvements are welcome ;-)

marcoow commented 9 years ago

@AW-UC: instead of reopening the session (I think reopen in general is a code smell) you could simply define an initializer that injects the store into the session. That way you could simply do this.store.find('user', userId) instead of this.container.lookup('store:main').find('user', userId).

Awem commented 9 years ago

@marcoow yeah, I am totally unhappy with reopen as well, but I haven't found any other way to make it work in an instance initializer. Injecting services doesn't seem to work well in an instance initializer (still buggy?), but of course I could use a normal initializer:

// app/initializers/custom-session.js
import Ember from 'ember';
import DS from 'ember-data';
import Session from 'simple-auth/session';

function initialize(container, application) {
  var CustomSession = Session.extend({
    storeService: Ember.inject.service('store'),
    currentUser: Ember.computed('secure.user_id', function () {
      var userId = this.get('secure.user_id');
      if (!Ember.isEmpty(userId)) {
        return DS.PromiseObject.create({
          promise: this.get('storeService').find('user', userId)
        });
      }
    })
  });
  application.register('session:custom', CustomSession);
}

export default {
  name: 'custom-session',
  after: 'ember-data',
  initialize: initialize
};

But then I get the 'lookup' was called on a Registry deprecation again :-(

marcoow commented 9 years ago

You can just add another initializer and define an injection of store into session:

application.inject('session:custom', 'modelStore', 'store:main');
Awem commented 9 years ago

@marcoow thanks, it works well. but only, if it is not injected as store, because the session already has a store property.

marcoow commented 9 years ago

@AW-UC right, forgot about that - fixed in above comment

ludalex commented 9 years ago

:+1:

Awem commented 9 years ago

FYI, this how the ember-intl addon handled this:

  1. Split initializers: https://github.com/yahoo/ember-intl/commit/4ad7c4094c753547b1e863adaac7b8a417d568d9
  2. Fallback: https://github.com/yahoo/ember-intl/commit/ad3c8337a5a24f103fa46f2ecc2d5a6ed34d3163
marcoow commented 9 years ago

I think we can actually make that part of #522 and simply require a newer Ember version then given the fact that instance initializers also work in Ember versions prior to 2.0

brandonparsons commented 9 years ago

Hi @marcoow - what is your recommendation for how to handle this ? (i.e. ignore deprecations for now, custom session, wait for 522?)

marcoow commented 9 years ago

@brandonparsons: I think it should be part of #522. I won't have that ready until 2.0 beta 1 comes out so any help with that would be welcome!

jeremytm commented 9 years ago

:+1:

juggy commented 9 years ago

My latest PR fixes this and is backward compatible.

brandonparsons commented 9 years ago

@juggy @marcoow Is your PR good to use until #522 lands?

mmahalwy commented 9 years ago

@juggy any update on the PR?

juggy commented 9 years ago

Yes you can use PR #560 until #522 is done.

mmahalwy commented 9 years ago

Awesome! Thank you!

injaon commented 9 years ago

:+1:

marcoow commented 9 years ago

fixed in #522

NullVoxPopuli commented 9 years ago

was there ever a consensus on how to set currentUser? and access it from {{session.currentUser}} in a template?

I currently have this:

// app/instance-initializers/custom-session.js

import Ember from 'ember';
import DS from 'ember-data';

function initialize(application) {
  var session = application.container.lookup('simple-auth-session:main');
  var store = application.container.lookup('store:main');

  session.reopen({
    currentUser: Ember.computed('secure.token', function() {
      var token = this.get('secure.token');
      if (!Ember.isEmpty(token)) {
        return DS.PromiseObject.create({
          /*
            the id of 1 here doesn't actually matter,
            the server always returns the current user.
            This is just to route to the show action on the controller.
          */
          promise: store.find('user', 1)
        });
      }
    })
  });
}

export default {
  initialize: initialize
};

The user is retrieved, but {{session.currentUser}} is empty.

:-\

vmalloc commented 9 years ago

I think session.secure is now session.data.authenticated. Replace that and you should be fine. On שבת, 3 באוק׳ 2015 at 22:10 L. Preston Sego III notifications@github.com wrote:

was there ever a consensus on how to set currentUser? and access it from {{session.currentUser}} in a template?

I currently have this:

// app/instance-initializers/custom-session.js

import Ember from 'ember';import DS from 'ember-data';

function initialize(application) { var session = application.container.lookup('simple-auth-session:main'); var store = application.container.lookup('store:main'

);

session.reopen({ currentUser: Ember.computed('secure.token', function() { var token = this.get('secure.token'); if (!Ember.isEmpty(token)) { return DS.PromiseObject.create({ /* the id of 0 here doesn't actually matter, the server alwasy returns the current user. This is just to route to the show action on the controller. */ promise: store.find('user', 1) }); } }) }); } export default { initialize: initialize };

The user is retrieved, but {{session.currentUser}} is empty.

:-\

— Reply to this email directly or view it on GitHub https://github.com/simplabs/ember-simple-auth/issues/517#issuecomment-145279507 .

NullVoxPopuli commented 9 years ago

I don't know if that's correct.

when I have "secure.token", the request is made, but when I do session.data.authenticated, no request is made to obtain the user.