-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
ES5 Getters #281
Merged
Merged
ES5 Getters #281
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,180 @@ | ||
- Start Date: 2017-12-12 | ||
- RFC PR: https://github.com/emberjs/rfcs/pull/281 | ||
- Ember Issue: (leave this empty) | ||
|
||
# Summary | ||
|
||
Install ES5 getters for computed properties on object prototypes, thus | ||
eliminating the need to use `this.get()` or `Ember.get()` to access them. | ||
|
||
Before: | ||
|
||
```js | ||
import Object, { computed } from '@ember/object'; | ||
|
||
const Person = Object.extend({ | ||
fullName: computed('firstName', 'lastName', function() { | ||
return `${this.get('firstName')} ${this.get('lastName')}`; | ||
}) | ||
}); | ||
|
||
let chancancode = Person.create({ firstName: 'Godfrey', lastName: 'Chan' }); | ||
|
||
chancancode.get('fullName'); // => 'Godfrey Chan' | ||
|
||
chancancode.set('firstName', 'ʎǝɹɟpo⅁'); | ||
|
||
chancancode.get('fullName'); // => 'ʎǝɹɟpo⅁ Chan' | ||
|
||
let { firstName, lastName, fullName } = chancancode.getProperties('firstName', 'lastName', 'fullName'); | ||
``` | ||
|
||
After: | ||
|
||
```js | ||
import Object, { computed } from "@ember/object"; | ||
|
||
const Person = Object.extend({ | ||
fullName: computed('firstName', 'lastName', function() { | ||
return `${this.firstName} ${this.lastName}`; | ||
}) | ||
}); | ||
|
||
let chancancode = Person.create({ firstName: 'Godfrey', lastName: 'Chan' }); | ||
|
||
chancancode.fullName; // => 'Godfrey Chan' | ||
|
||
chancancode.set('firstName', 'ʎǝɹɟpo⅁'); | ||
|
||
chancancode.fullName; // => 'ʎǝɹɟpo⅁ Chan' | ||
|
||
let { firstName, lastName, fullName } = chancancode; // No problem! | ||
``` | ||
|
||
# Motivation | ||
|
||
Ember inherited its computed properties functionality from [SproutCore](http://guides.sproutcore.com/core_concepts_kvo.html). | ||
The feature was designed at a time before [ES5 getters](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get) | ||
were widely available. This necessitated using a special function such as | ||
`this.get()` or `Ember.get()` to access the values of computed properties. | ||
|
||
Since all of our [target browsers](https://github.com/emberjs/rfcs/pull/252) | ||
support ES5 getters now, we can drop the need of this special function, | ||
improving developer egornomics and interoperability between other libraries | ||
and tooling (such as TypeScript). | ||
|
||
Note that at present, using `this.set()` or `Ember.set()` is still mandatory | ||
for the property to recompute properly. In the future, we might be able to | ||
loosen this requirement, perhaps with the help of ES5 setters. However, that | ||
would require more design and is out-of-scope for this RFC. | ||
|
||
`this.get()` and `Ember.get()` will still work. This RFC does not propose | ||
removing or deprecating them in the near term. They support other use cases | ||
that ES5 getters does not, such as "safe" path chaining (`get('foo.bar.baz')`) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the curious: Babel 7 will support optional chaining. |
||
and `unknownProperty` (and Proxies by extension), so any future plans to | ||
deprecate them would have to take these features into account. | ||
|
||
Addon authors would likely need to continue using `Ember.get()` for at least | ||
another two LTS cycle (8 releases) to support older versions of Ember (and | ||
possibly longer to support proxies). It is however very unlikely that the | ||
everyday user would need to use this. | ||
|
||
# Detailed design | ||
|
||
The computed property function, along with any caches, can be stored in the | ||
object's "meta". We will then define a getter on the object's prototype to | ||
compute the value. | ||
|
||
One caveat is that the computed property function is currently stored on the | ||
instances for implementation reasons that are no longer relevant. However, | ||
it is possible that some developers have observed their existance and have | ||
accidentaly relied on these private semantics (e.g. `chancancode.fullName.get()` | ||
or `chancancode.fullName.isDescriptor`). | ||
|
||
Before landing this change, we should turn the property into an assertion | ||
for so that in these unlikely scenarios the developers will at least receive | ||
some warning. | ||
|
||
Another thing to consider is that there is this Little Known Trick™ to add | ||
Computed Properties to POJOs: | ||
|
||
```js | ||
import { computed, get } from "@ember/object"; | ||
|
||
let foo = { | ||
bar: computed(function() { return 'bar'; }) | ||
}; | ||
|
||
get(foo, 'bar'); // => 'bar' | ||
``` | ||
|
||
In this case, there is no opportunity for us to install an ES5 getter, and | ||
`Ember.get` is the only solution. This is very rare in practice and is more | ||
or less just a party trick. We should deprecate this use case (in `Ember.get`) | ||
and suggest the alternative: | ||
|
||
```js | ||
import Object, { computed } from "@ember/object"; | ||
|
||
let foo = Object.extend({ | ||
bar: computed(function() { return 'bar'; }) | ||
}).create(); | ||
|
||
foo.bar; // => 'bar' | ||
``` | ||
|
||
Or simply... | ||
|
||
```js | ||
let foo = { | ||
get bar() { | ||
return 'bar'; | ||
} | ||
}; | ||
|
||
foo.bar; // => 'bar' | ||
``` | ||
|
||
# How We Teach This | ||
|
||
For the most part, this RFC _removes_ a thing that we need to teach new | ||
users. | ||
|
||
It might, however, come across as slightly strange that `set()` is still | ||
required. However, many other libraries share the same model and | ||
empricially this does not appear to be an issue. For example, in React, | ||
you can freely access `this.state.foo` but must use `this.setState('foo', ...)` | ||
to update it. Even Vue has [the same API](https://vuejs.org/v2/api/#Vue-set) | ||
for some cases. | ||
|
||
The mental model for this is that you must use the `set()` in order for | ||
Ember to notice your mutations, so that it can update the caches, rerender | ||
things on the screen, etc. | ||
|
||
As for users who already learned to use `get()` everywhere, that would | ||
continue to work. Ideally, this would be a Cool Trick™ they pick up some day | ||
(as in "Oh, I don't have to do _that_ anymore? Cool."), at which point the | ||
old habbit would quickly die. If this turned out to be too confusing, we | ||
could always explore deprecating `this.get()`; we will just have to weight | ||
the cost-benefits of the confusion (if any) vs churn. | ||
|
||
# Drawbacks | ||
|
||
As mentioned, not removing `set()` at the same time might be a source of | ||
confusion. However, removing `set()` would require significantly more | ||
upfront design work, and it [might not even be possible](https://vuejs.org/v2/guide/reactivity.html#Change-Detection-Caveats) | ||
to completely remove the need of `set()` (as the system is designed today) | ||
in all cases (see `Vue.set()`). | ||
|
||
Since removing `get()` would unlock so many benefits, and since there are | ||
plenty of other libraries that uses the same model, the case for decoupling | ||
the two seems overwhemlingly positive. | ||
|
||
# Alternatives | ||
|
||
* Hold off until we also remove `set` | ||
* Hold off until we transition to something like [Glimmer's `@tracked`](https://glimmerjs.com/guides/tracked-properties) | ||
|
||
In my opinion, these alternatives do not make a lot of sense, as neither | ||
of these hypothetical systems appear to require (or would benefit from) | ||
having a user-land getter system. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it should be said here. But we can improve ergonomics of tooling such as TypeScript today with
Ember.get
. And if we will able to pass an Array toEmber.get
likeEmber.get(["some", "nested", "key"])
we would even have better ergonomics with TSThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried in the past, and I am pretty convinced you cannot type an Ember object successfully today, even without considering nesting. You need to convince TypeScript that they are no properties on the object itself (so obj.foo etc are type errors) but when you do
obj.get(“foo”)
orget(obj, “foo”)
it should have the right type. I have come close with using some phantom associated type tricks but I think it’s fundamentally impossible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what it’s worth, you can, and we are... as long as you don’t do nested properties. It took a fairly ridiculous amount of work, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be precise: the types you end up with on the object itself end up having a type of
ComputedProperty<TheType>
, which the type system then properly unwraps when usingget
.