Relying on unknownProperty breaks can helper

[oakho]

First of all, thanks for writing this addon, we are relying a lot on it in our app.

While upgrading to the latest version of ember-can we came across an issue that currently breaks our app.

We rely on the unknownProperty of EmberObject to match abilities with a preloaded list we get from our backend to avoid defining all of them by hands.

To give you some insight, here's our base ability class :

export default class Base extends Ability {
  @alias('session.currentUser') user;
  @alias('session.currentDistributor') distributor;

  hasAbility(key) {
    let user = this.get('user');
    return user ? user.hasAbility(camelize(key.replace('can', '')), this.get('subject')) : false;
  }

  unknownProperty(key) {
    return this.hasAbility(key);
  }
}

Unfortunately, there was a change introduced in 2d8a197#diff-6ef2539ada25eeefeef778ae6f0aef826e9bf2ba2e8b94cf138f1ea70267de2dR21, that breaks the helper in our case.

We get the error :

Uncaught (in promise) Error: Assertion Failed: You attempted to access the `canManage` property (of <idol@ability:product::ember524>).
Since Ember 3.1, this is usually fine as you no longer need to use `.get()`
to access computed properties. However, in this case, the object in question
is a special kind of Ember object (a proxy). Therefore, it is still necessary
to use `.get('canManage')` in this case.

This obviously happens because of our use of unknownProperty while ember-can doesn't use ember's get method to retrieve the abilities properties, removing it fixes the error.

I understand that you removed the use ember's get method to support native class but our issue can easily be fixed by checking if the ability inherits EmberObject like so :

if(ability instanceof EmberObject) {
  return ability.get(propertyName);
}

return ability[propertyName];

I can open a PR with our fix if this is something you'd be okay with.

[Exelord]

Hi,
Thank you for raising this up! That indeed could be a solution, however, I'm not sure If we want to support EmberObject further (open on others' voices) as it has been kind of deprecated.

Another solution for your case could be defining abilities in the constructor if you are able to get all the keys. I usually follow this path. So instead of relaying on unknownProperty you could do:

abilities.forEach((key) => {
  Object.defineProperty(this, key, { get() { return this.hasAbility(key) } })
})

Let me know If that would solve your issue :)

[oakho]

Hello @Exelord,

I agree with you that with the soft deprecation of EmberObject this might not be the best solution but then I think there might be some confusion for other users with the current state of ember-can.

Currently, the ability class still extends EmberObject itself so as a user I would expect to be able to rely on public APIs exposed by EmberObject without breakings.

From my point of view, ember-can should either get rid of EmberObject and release a major release with breaking changes (this may also resolve #92) or stay backward compatible with features supported by EmberObject.

What do you think ?

That said, I understand this is an open source project and you might not want to do that for a small edge case you didn't encounter.

Regarding your solution (thanks for it), it is in fact what I've done first, this works pretty well and I also think It's the go to solution to not rely on EmberObject. But, in our case, we also sometimes override the unknownProperty in our abilities' subclasses and that would involve a refactor that is out of scope of my initial work to get our app up to date and octane ready.

Anyway, I'm totally cool with keeping my can helper override until we can afford to spend some time to refactor our abilities with your solution :) !

[Exelord]

How about if we would use Ember.get(ability, key) :) Would that solve your issue?

[oakho]

Yes, using return get(ability, propertyName); would indeed fix my issue :) ! (Quite better than my initial solution).

I can open a PR, if you like !

[Exelord]

Ok, cool! I will accept it :)

[RobbieTheWagner]

@Exelord can this be closed? It looks like a PR was merged.