Deprecate old API #79

Exelord · 2018-04-05T21:02:36Z

This PR will deprecate old fashion API. The deprecations will stay until v2.0.0

Deprecations

You will not longer be able to import CanMixin as it will be removed. You can reproduce its behavior using can service and its methods.
import { CanService } from 'ember-can' -> import CanService from 'ember-can/services/can'
import { ability } from 'ember-can' -> import { ability } from 'ember-can/computed'
build method of can service will be replaced by abilityFor

Notable changes

You have to pass a null model if you want to pass only parameters to can or cannot methods of can service. Eg: this.get('canService').can('post', null, { test: true})
The model passed to helpers or can service methods, as from now on, can be anything.

Exelord · 2018-04-05T21:04:27Z

@sukima I need your eyes on this 🙏

Exelord · 2018-04-05T21:25:53Z

README.md

+    <img src="https://badge.fury.io/js/ember-can.svg"/>
+  </a>
+
+  <a href="https://travis-ci.org/minutebase/ember-can" title="Ember Observer">


https://emberobserver.com/addons/ember-can

miguelcobain · 2018-04-05T22:23:46Z

@Exelord I really like the plan you outlined. 👍

However, I still think that providing a computed macro has a lot of value.

Basically instead of doing:

can: service(), // inject can service
postAbility: computed('post', function() {
  return this.get('can').abilityFor('post', this.get('post'));
})

you could do

postAbility: ability('post', 'post')

Computed macros are pretty common in ember and many addons provide collections of them, as you may know.

We could import the macro using

import { ability } from 'ember-can';

but I saw in the original issue that you're concerned about accidentally importing the computed macro instead of the Ability class and vice-versa.

We could change the import path or the export name:

import { ability } from 'ember-can/macros';
// or
import { abilityMacro } from 'ember-can';
// other options
import { can } from 'ember-can';
import { a } from 'ember-can';

I think they are inferior to the first one because I personally don't think the accidental import is a big problem.
I think people know that js variables are case sensitive and capitalized vars are usually classes.

sukima · 2018-04-06T02:37:06Z

Will look more at this tomorrow. But so far I am liking the plan.

acorncom · 2018-04-06T04:27:41Z

I'd agree with @miguelcobain that it'd be nice to keep a computed macro around, makes the API nice and legible ...

Exelord · 2018-04-06T11:39:55Z

In my opinion using:

postAbility: computed('post', function() {
  return this.get('can').abilityFor('post', this.get('post'));
})

is much more explicit and more readable way, especialy that it's easily customizable in case when we want to pass custom properties: return this.get('can').abilityFor('post', this.get('post'), { value: true });

The use case of this method should be very rare, anyway.

But OK :) If you like it so much, let's have it for now and discuss it later on- when the time will come.
To have better Public API I suggest it to import it by using:

import { ability } from 'ember-can/computed';

to stay consistent with Ember computed imports eg:

import { reads } from '@ember/object/computed';

What do you think guys?

sukima · 2018-04-07T01:00:17Z

import { ability } from 'ember-can/computed';

LGTM!

And yes the explicit argument is well made and I think it is important that users can always use that option. As for the macro I say keep it in the API; we can always deprecate it later.

Exelord · 2018-04-17T20:48:32Z

Can someone do the last check here? :)

sukima · 2018-04-18T13:51:25Z

README.md

+
+  user: computed('session.currentUser', function() {
+    return this.get('session.currentUser');
+  }),


If we want to show best practices these should be:

user: reads('session.currentUser'), canCreate: reads('user.isAdmin')

We should even make it read-only:

user: reads('session.currentUser').readOnly(), canCreate: reads('user.isAdmin').readOnly()

But my purpose here was to explain the use case for everyone even for the beginners which may be not so familiar how the macros work exactly. Of course, the advanced developers will know how to improve it ;)

What do u think? Is it a good point?

Why readOnly? What nefarious reason do we need to prevent a developer from setting a user?

If we attempt to test this and want to change the property of canCreate we cannot instead we would have to go into the source see that it is a read only property from user and attempt to set user oh but we can't because that too is read only so now we have to mock out session and set its currentUser with an isAdmin property all so we can manipulate canCreate. I fail to see the benifit for telling the world never ever ever try to set this property to something you really think it should be

The only use cases I've found for read only is in cases where the integrity of the class is dependent on the calculations done in a getter. For example in ember-data setting to the models' attributes would be problematic and so it makes sense it is a read only value. Where as in this case these are aliases not a derived values. They are not calculated, they are used.

Keep in mind that this is just an example. But here are my thoughts.

Abilities shouldn't be overriden. That can break a lot. We can't have in one place the ability canCreate -> true and in the other false for the same model even if we want to. The readOnly will help to keep integrity between those states.

Second thing. We don't write code for easier testing. And even in tests, we shouldn't override the ability, just set the isAdmin or (more PRO) mock it using eg. Sinon.js.

I don't want to teach people here why they should (or not) use it, that's why I didn't add it. But as you know reads has some disadvantages and can mislead the expected behavior. More about it, eg. here: https://m.alphasights.com/the-risks-you-take-when-you-use-ember-computed-oneway-f2c008a14404

BTW. Im always making the property readOnly if I don't expect it to be ever overridden. Like using const and let. And I don't expect to override any ability on the fronted, as I'm not in charge to control what I can or not. That's a backend part. If you are creating abilities which are not dependent on the API and used only eg for display sth on UI, then you may want to set it on your own. In that case, there is no other choice than NOT to use readOnly.

Abilities shouldn't be overriden. That can break a lot. We can't have in one place the ability canCreate -> true and in the other false for the same model even if we want to. The readOnly will help to keep integrity between those states.

Ok, in this instance your argument has convinced me. Thank you.

Im always making the property readOnly if I don't expect it to be ever overridden.

However, I do disagree with this. We have a dev on our team that is vehement about adding readOnly to everything and I've had many WTF moments and lost time trying to understand why things break. It has been my experience that it is better to be permissive then it is to introduce weird errors and unexpected crashes because someone somewhere felt they knew the future better then I. Personal opinion obviously.

I prefer to stick with the default for most cases and add a readOnly after I've convinced myself that changes to that property would be bad. As in your example above about abilities it is arguably true that changing an ability property manually would not be beneficial and readOnly makes sense there.

Thank you for the time to explain your perspective. It has helped me learn something new today.

You are definitely right. We shouldn't add it everywhere we can. We have to be very careful with it. Of course, your experience will help you a lot with it, so with time, you will feel which are readOnly and which are not. The same as with consts. That requires some skill. But trust me there are some cases when this mark can save you a lot of trouble. You have to just use it wisely and exactly know the purpose. Not by guessing ;)

At the beginning of the Ember journey, I totally agree and do not recommend to use it until the moment you will realize that you really need it right now. Then you will be ready to control the force 😅

No problem. That's what for the community is. Good to know your concerns as well :)

Anyway, you still think we should change the example to reads?

I think the reads are good because most newbies don't realize they are there

sukima · 2018-04-18T13:53:51Z

addon/computed.js

-    });
-  }
-};
+  return computed(resourceName, function() {


We should either support or document the use case where we have more then just the resource to deal with. Some abilities might have need for named properties.

sukima · 2018-04-18T13:57:20Z

addon/computed.js

-    });
-  }
-};
+  return computed(resourceName, function() {


We should either support or document the use case where the ability needs named properties (not just a resource) to work.

miguelcobain · 2018-04-18T13:58:11Z

UPGRADING.md

@@ -4,6 +4,11 @@ Whilst experimenting with the API & benefiting from changes in Ember, we've had

 Here are the details on updating from previous versions.

+## From v2.0.0
+1. You will not longer be able to import `CanMixin` and `computed` as they will be removed. You can reproduce theirs behaviours using `can` service and its methods.


This seems to be inaccurate as the computed macro still exists. It's just imported differently.

Some old stuff :/

Exelord · 2018-04-23T19:19:59Z

This is last chance to talk about your thoughts and concerns according to this PR. 🤔➡👌⭐️

Don't bother to mark this comment as 👍 if you are ok to merge it.

miguelcobain · 2018-04-23T19:26:55Z

UPGRADING.md

+
+### Notable changes
+1. You have to pass a `null` model if you want to pass only parameters to `can` or `cannot` methods of `can` service. Eg: `this.get('canService').can('post', null, { test: true})`
+2. The `model` passed to helpers or `can` service methods, as from now on, can be anything.


I wasn't aware that there were any restrictions earlier, to be honest. Looking at the changes it seems like we could only use Ember.Object instances before. Perhaps we should make that clear using:

The model passed to helpers or can service methods, as from now on, can be anything instead of an Ember.Object instance.

This isn't super important, by any means. I'll leave this to your consideration.

Again, great job on this.

miguelcobain · 2018-04-23T19:27:19Z

UPGRADING.md

+4. `build` method of `can` service will be replaced by `abilityFor`
+
+### Notable changes
+1. You have to pass a `null` model if you want to pass only parameters to `can` or `cannot` methods of `can` service. Eg: `this.get('canService').can('post', null, { test: true})`


My OCD is telling me that we need a space after true.

Deprecate old API

ee60154

Exelord added the enhancement label Apr 5, 2018

Exelord commented Apr 5, 2018

View reviewed changes

This was referenced Apr 5, 2018

Question about the necessity of having an Ember.Object as a resource #57

Closed

provide ability computed macro as a named export #70

Closed

Exelord added 3 commits April 11, 2018 15:31

Update package lock

3d3b139

Apply feedback

731d989

Merge branch 'master' into feature-deprecate-api

36e76e9

sukima reviewed Apr 18, 2018

View reviewed changes

miguelcobain reviewed Apr 18, 2018

View reviewed changes

Apply feedback

7975f6f

Fix deprecated computed import

9065c66

miguelcobain reviewed Apr 23, 2018

View reviewed changes

Apply feedback

0328f06

Exelord merged commit 0e58e24 into master Apr 26, 2018

Exelord deleted the feature-deprecate-api branch April 26, 2018 08:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate old API #79

Deprecate old API #79

Exelord commented Apr 5, 2018 •

edited

Loading

Exelord commented Apr 5, 2018 •

edited

Loading

Exelord Apr 5, 2018

miguelcobain commented Apr 5, 2018

sukima commented Apr 6, 2018 •

edited

Loading

acorncom commented Apr 6, 2018

Exelord commented Apr 6, 2018 •

edited

Loading

sukima commented Apr 7, 2018

Exelord commented Apr 17, 2018

sukima Apr 18, 2018

Exelord Apr 18, 2018

sukima Apr 18, 2018

Exelord Apr 18, 2018

sukima Apr 18, 2018

Exelord Apr 18, 2018 •

edited

Loading

sukima Apr 18, 2018

sukima Apr 18, 2018

sukima Apr 18, 2018 •

edited

Loading

miguelcobain Apr 18, 2018

Exelord Apr 18, 2018

Exelord commented Apr 23, 2018

miguelcobain Apr 23, 2018

miguelcobain Apr 23, 2018

Deprecate old API #79

Deprecate old API #79

Conversation

Exelord commented Apr 5, 2018 • edited Loading

Deprecations

Notable changes

Exelord commented Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

miguelcobain commented Apr 5, 2018

sukima commented Apr 6, 2018 • edited Loading

acorncom commented Apr 6, 2018

Exelord commented Apr 6, 2018 • edited Loading

sukima commented Apr 7, 2018

Exelord commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Exelord Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sukima Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Exelord commented Apr 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Exelord commented Apr 5, 2018 •

edited

Loading

Exelord commented Apr 5, 2018 •

edited

Loading

sukima commented Apr 6, 2018 •

edited

Loading

Exelord commented Apr 6, 2018 •

edited

Loading

Exelord Apr 18, 2018 •

edited

Loading

sukima Apr 18, 2018 •

edited

Loading