-
-
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
Deprecate computed().volatile()
#370
Merged
Merged
Changes from all commits
Commits
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,123 @@ | ||
- Start Date: 2018-08-31 | ||
- RFC PR: (leave this empty) | ||
- Ember Issue: (leave this empty) | ||
|
||
# Summary | ||
|
||
Deprecate `computed().volatile()` in favor of undecorated native getters and | ||
setters. | ||
|
||
# Motivation | ||
|
||
`computed().volatile()` is a commonly misunderstood API. On its surface, | ||
declaring a computed as volatile causes the computed to recalculate every time | ||
it is called. This actually works much like native, undecorated accessors do on | ||
classes, with one key difference. | ||
|
||
Volatile properties are meant to respresent fundamentally unobservable values. | ||
This means that they swallow notification changes, and will not notify under any | ||
circumstances, and that when setting a volatile value the user must notify | ||
manually: | ||
|
||
```js | ||
const Foo = EmberObject.extend({ | ||
bar: computed({ | ||
get() { | ||
return this._value; | ||
} | ||
|
||
set(key, value) { | ||
return this._value = value; | ||
} | ||
}).volatile(), | ||
|
||
baz: computed('bar', { | ||
get() { | ||
return this.bar; | ||
} | ||
}), | ||
}); | ||
|
||
let foo = Foo.create(); | ||
|
||
foo.set('bar', 123); | ||
foo.baz; // 123, it's the initial get so nothing cached yet | ||
|
||
foo.set('bar', 456); | ||
foo.baz; // 123, no property changes were made so the cache was not cleared | ||
``` | ||
|
||
This behavior is useful at times for framework code, but is generally not what | ||
users are expecting. By constrast, when using native accessors with `set` and | ||
`get`, Ember treats them just like any other property. From its perspective, | ||
they _are_ standard properties, so it'll continue to notify as expected. | ||
|
||
```js | ||
class Foo { | ||
get bar() { | ||
return this._value; | ||
} | ||
|
||
set bar(value) { | ||
this._value = value; | ||
} | ||
|
||
@computed('bar') | ||
get baz() { | ||
return this.bar; | ||
} | ||
}); | ||
|
||
let foo = new Foo(); | ||
|
||
set(foo, 'bar', 123); | ||
foo.baz; // 123, it's the initial get so nothing cached yet | ||
|
||
set(foo, 'bar', 456); | ||
foo.baz; // 456, cache was cleared and value was updated | ||
``` | ||
|
||
The most common use case for volatile computeds was when users wanted a computed | ||
to behave like a native getter/setter. Now that we (almost) _have_ those in a | ||
easy to use form, it makes more sense to deprecate the volatile API and rely | ||
directly on native functionality. | ||
|
||
# Transition Path | ||
|
||
Native getters and setters will _only_ work on native classes, due to how the | ||
internals of the old object model work. To ensure that users do not accidentally | ||
try to replace volatile with getters/setters on non-native classes, we should | ||
provide 2 deprecation warnings: | ||
|
||
1. Deprecation when users use volatile on a computed which tells them that the | ||
API has been deprecated, and that they'll need to update native class syntax | ||
to remove the volatile property. | ||
|
||
2. Deprecation when users use volatile on a computed decorator (to be RFC'd) | ||
which tells them to remove the computed decorator entirely from the getter. | ||
|
||
Volatile properties will be removed once native classes are the default. | ||
|
||
# How We Teach This | ||
|
||
In general documentation should be updated to use native getters and setters | ||
wherever `volatile` was used. This will have to happen after docs are updated to | ||
use native classes, because native getters and setters do _not_ work with the | ||
older object model. | ||
|
||
# Drawbacks | ||
|
||
Volatility is useful for framework level concerns, for instance if developing an | ||
API or decorator that already handles notification. Addon authors may be able to | ||
use this functionality. | ||
|
||
Not having an alternative for old style classes or mixins could be problematic | ||
for users who aren't ready to update to native class syntax. | ||
|
||
# Alternatives | ||
|
||
We could keep `volatile()` around for any potential addons that may want to use | ||
it, but teach native getters/setters as the preferred path for most use cases. | ||
|
||
We could provide `volatile` as a separate API/decorator to distinguish it from | ||
computed properties, and discourage use for users. |
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.
TODO: update this to clarify that getter/setter based properties with
Object.defineProperty
still work with older versions ofEmber.Object