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

Gathering feedback: Support native ES properties for computed properties #108

Closed
chrisjshull opened this issue Dec 14, 2015 · 15 comments
Closed

Comments

@chrisjshull
Copy link

This issue is intended to gather feedback on making computed properties gettable and settable via native properties (in addition to the get/set methods).

For example, instead of:

myObj.get('computedProperty');
myObj.set('computedProperty', 1234);

You could do:

myObj.computedProperty;
myObj.computedProperty = 1234;

Benefits:

  • No more issues when a native property is swapped out with a computed property, requiring all API consumers to switch to get/set.
    • Alt: no more defensively coding with get/set all the time "just in case". *
  • Better autocomplete/discoverability in REPLs and IDEs (and hover support in Web Inspector).
  • Less typing.
  • More "natural" JS.

* Even if you do this, it can't be applied across the board since you still have to deal with native/non-Ember objects which don't have get/set.

Drawbacks:

  • unknownProperty obviously won't work if you don't actually call get/set. unknownProperty is rarely used - classes tend to declare all of their actual properties. However, if your object actually takes arbitrary properties (like an ES Map), get and set will still be there for you.

Again, not proposing the removal/deprecation of get/set. If you like the methods better, or have use of unknownProperty then they will still be there for you. But I think it would be nice to use native accessors for the majority of cases where you don't even want unknownProperty.

@locks
Copy link
Contributor

locks commented Dec 14, 2015

Less typing

Not an argument.

Again, not proposing the removal/deprecation of get/set.

Why not?

@grapho
Copy link

grapho commented Dec 14, 2015

i like the idea of just: Ember.get(myObj, 'myVal')/Ember.set(myObj, 'myVal', 'foo').. if you are concerned about native/computed properties.. ember/non-ember objects.

@chrisjshull
Copy link
Author

Less typing

Not an argument.

Why not? Given two lines that function the same and are ~equally grokkable I'll happy take the shorter one.
But granted, not the most important argument.

Again, not proposing the removal/deprecation of get/set.

Why not?

Still has some value for a tiny, specific set cases, like if you want to make a Map-like object.

I like the idea of just: Ember.get(myObj, 'myVal')/Ember.set(myObj, 'myVal', 'foo').. if you are concerned about native/computed properties.. ember/non-ember objects.

But I don't think you should have to worry about it at all. It's still a trap.
And JS already has accessors - why not leverage them?

@jonnii
Copy link

jonnii commented Dec 14, 2015

For the get case this is (in my opinion) a no-brainer, there is however value in explicitly calling set as it makes changes to state more obvious. Also, wasn't this already discussed around the decision to drop ie8/9 compatibility?

@chrisjshull
Copy link
Author

Hi, @jonnii,

there is however value in explicitly calling set as it makes changes to state more obvious

Can you expound on this?
I'm not sure how:

myObj.set('computedProperty', 1234);

Is more obvious than:

myObj.computedProperty = 1234;

Assignment is assignment, no? (I, myself, read these the same.)

@stefanpenner
Copy link
Member

the get case is planned to happen ever since we dropped ie8 (Everyone is on board, work just needs to be done), just requires a multi-release process, as descriptors as emberjs/ember.js#10323 accidentally opened leaked descriptors into the object...... This will need to be deprecated (to be nice to those that have begun relying on it)

quick example of the issue we will need to deprecate (and why i haven't already just done get -> ES5 descriptor)

init() {
  this._super(...arguments);
  this.foo = Ember.computed.alias('bar') // this due to #10323 makes `foo` a descriptor alias to bar...
}

The set will need more exploration, but I believe as @jonnii mentioned the current thought is to leave the explicit set, to make it clear set is for change tracking, not just assignment.

@jonnii
Copy link

jonnii commented Dec 14, 2015

@chrisjshull a better example would be setProperties.

@chrisjshull
Copy link
Author

Another reason to keep around get/set methods even with native accessors:
The getPath/setPath abilities of get/set are quite useful, in cases where you actually want undefined out of "broken" paths. Saves a lot of typing vs checking for null/undefined in each part of the path.

@stefanpenner, for set would native assignment also be supported in that current thought?
(If not, it would seem awkwardly non-parallel to support native get without native set.)

@stefanpenner
Copy link
Member

Another reason to keep around get/set methods even with native accessors:

we have no plans on removing get/set (needlessly removing things that would break apps, seems poor choice, and proxies/paths must use the get approach anyways) but allowing for native ES5 descriptor variant of get

, for set would native assignment also be supported in that current thought?

I believe the current thought, is to only support explicit set variant, to obviously say "this is being changed, and it should be tracked". This is something @wycats has mentioned, before I speak for him, he should likely be the one to response. (pinged him in chat)

@chrisjshull
Copy link
Author

If indeed the thought is that native get would be supported, but not native set, I can only see that being super confusing due to lack of parallelism, leading to more hard-to-diagnose issues.

Moreover, it's forcing what is potentially an internal concern onto API consumers. Let's say I create an API with a property foo. Internally whenever that property changes I need to make sure I do something. Why must the API consumer be aware of that fact? It's not the API consumer who needs to make the call of "should this be tracked", but the API's internals.

And you still have the upgradability issue, where foo is a plain old property and everyone is safe to use native getters and setters. Until the day I need to add that observer or dependent computed property and have to break all of those consumer's setters.

@chrisjshull
Copy link
Author

@wycats, sounds like your thoughts were desired...?

@stefanpenner
Copy link
Member

I have some work that does this, but performance is a concern. Once that is completed (35 remaining failing tests), more thorough performance work will be completed at which point we can re-evaluate.

@adriaandotcom
Copy link

adriaandotcom commented Jun 23, 2018

In ember-cli/ember-cli-update#255 I figured Ember.js is now supporting getters with traditional object dot notation for computed properties.

So for other people coming here, this is now supported:

import Component from '@ember/component';
import { computed } from '@ember/object';

export default Component.extend({
  name: computed('firstName', 'lastName', function() {
    return `${this.firstName} ${this.lastName}`;
  }),

  message: computed('name', function() {
    return `Hello ${this.name}!`;
  })
});

emberjs.com/blog/2018/04/13/ember-3-1-released.html

No more get's and set's! #108 (comment)

@webark
Copy link

webark commented Jun 23, 2018

@adriaanvanrossum set’s are still needed.. :/ and some gets.. but ya!! It’s getting better!! And you’ll have to do less typing too!! 😁😜😇

@rwjblue
Copy link
Member

rwjblue commented May 30, 2019

As noted above, CP's can be used without Ember.get as of Ember 3.1 (assuming you aren't using unknown property), and soon on canary (~ Ember 3.13) CP setters will be allowed without using Ember.set.

Closing...

@rwjblue rwjblue closed this as completed May 30, 2019
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

No branches or pull requests

8 participants