-
-
Notifications
You must be signed in to change notification settings - Fork 405
RFC: Getter CP readOnly by default #12
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
Changes from all commits
ded28ef
2185c65
c69316b
1ed3f2e
efd2fbe
415cc0b
ce6ad7f
f340008
a4d92d5
0883d93
1ef68e7
c663575
414a9c7
58a973e
b07f76c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| - Start Date: 2014-19-30 | ||
| - RFC PR: (leave this empty) | ||
| - Ember Issue: (leave this empty) | ||
|
|
||
| # Summary | ||
|
|
||
| Getter only computed properties should be readOnly by default | ||
|
|
||
| # Motivation | ||
|
|
||
| Overridable CP's are unexpected, hard to learn, and wanted in the minority of | ||
| cases. When it happens unexpeditly it is nearly always unwanted and the resulting | ||
| chaos is hard to debug. | ||
|
|
||
|
|
||
| ```js | ||
| var User = Ember.Object.extend({ | ||
| isOld: Ember.computed('age', function() { | ||
| return this.get('age') > 30; | ||
| }); | ||
| }); | ||
|
|
||
| var user = User.create({ | ||
| age: 30 | ||
| }); | ||
|
|
||
| user.get('isOld'); // => false | ||
|
|
||
| user.set('age', 31); | ||
| user.get('isOld'); // => true | ||
|
|
||
| user.set('isOld', false); | ||
| user.get('isOld'); // => false | ||
|
|
||
| user.set('age', 30); | ||
| user.get('isOld'); // => false (EWUT?) | ||
| ``` | ||
|
|
||
|
|
||
| What just happened: | ||
|
|
||
| * although age > 30 : isOld remains false | ||
| * by explicitly setting isOld, we have diverged the CP to no longer invalidate based on its dependentKey. | ||
| * many bugs and hair loss | ||
| * no way to sensibly recover | ||
| * rarely wanted | ||
|
Member
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. I'd remove this line. I also think you're missing the original motivation, which was to encapsulate a pattern that was extremely common before we added this behavior, which was something like: Ember.Object.extend({
version: function(key, value) {
if (arguments.length >= 2) {
this._version = value;
return value;
} else if (this._version) {
return this._version;
} else {
return 1;
}
}.property()
})This kind of lazy default was actually reasonably common, and I don't think it's a use-case we should pretend is completely obscure and absurd.
Member
Author
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. As soon as someone adds a dependent key it does become absurd. Your example illustrates this, as adding a dependent key would correctly purge the cache if it invalidated, unfortunately today's behavior when adding a dependent key would not purge the cache and leave the CP in an unexpected state. |
||
|
|
||
|
|
||
| # Detailed design | ||
|
Member
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. You should be clear, up front, about the detailed design in |
||
|
|
||
| ```js | ||
| firstName: Ember.computed(function() { | ||
|
|
||
| }); // readOnly | ||
|
Member
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. You should explain what "read only" means; specifically the fact that it's an exception and not a black-hole. |
||
| ``` | ||
|
|
||
| Is equivalent to: | ||
|
|
||
| ```js | ||
| firstName: Ember.computed({ | ||
| get: function() { | ||
|
Member
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. You haven't described the fully verbose version yet so you can't really refer to this as being equivalent to it. I'd reorder this section to explain the full |
||
| }); // readOnly (same as above) | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ```js | ||
| firstName: Ember.computed(function() { | ||
|
|
||
| }).overridable(); // writable | ||
|
Member
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. You should explain exactly what |
||
| ``` | ||
|
|
||
| Is equivalent to: | ||
|
|
||
| ```js | ||
| firstName: Ember.computed({ | ||
| get: function() { | ||
| }).overridable(); // writable | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
|
|
||
| ```js | ||
| firstName: Ember.computed(function(key, value) { | ||
|
|
||
| }).overridable(); // throw new Error("Overridable cannot be used if a setter already exists...."); | ||
| ``` | ||
|
|
||
| Is equivalent to: | ||
|
|
||
| ```js | ||
| firstName: Ember.computed({ | ||
| get: function() { }, | ||
| set: function() { } | ||
|
Member
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. I thought I remembered us going with: firstName: Ember.computed({
+ get: function() { },
+ set: overridable
})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. I prefer what wycats wrote above, if this is the only way to create the clobbering setter there is no need for the assertion, if we add the other it should at least decompose to this. |
||
| }).overridable(); // throw new Error("Overridable cannot be used if a setter already exists...."); | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ```js | ||
| firstName: Ember.computed({ | ||
| get: function() { }, | ||
| set: function() { } | ||
| }).readOnly(); // throw new Error("Cannot mark a CP as readOnly if it has an explicit setter"); | ||
| ``` | ||
|
|
||
| ```js | ||
|
|
||
| firstName: Ember.computed({ | ||
| get: function() { } | ||
| }); // on super class | ||
|
|
||
| firstName: Ember.computed({ | ||
| get: function() { }, | ||
| set: function(key, value) { this._super(key, value); } | ||
| }); // throw new Error("ReadOnly Error blah blah blah"); | ||
| ``` | ||
|
|
||
| # migration: | ||
|
|
||
| * 1.x update internals to correctly use `writable` if needed (and runs tests against this) | ||
|
Member
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. This really needs to be fleshed out in more detail. I could write it up if you want?
Member
Author
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. sure, that would be great
Member
Author
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. @wycats ping? 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. @wycats this ended with "I could write it up if you want?" if you don't have time can we move forward in some way?
Contributor
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. I already started to implement this. The implementation is very simple in fact, but there is loads of failing tests. It requires some thought to decide which internal properties should be writable. |
||
| * 1.x deprecate writability unless `writable` was explicitly called, or new `set` syntax was used | ||
| * 2.0 flip the switch to `readOnly` by default and never look back. | ||
|
|
||
| # Drawbacks | ||
|
|
||
| N/A | ||
|
|
||
| # Alternatives | ||
|
|
||
| N/A | ||
|
|
||
| # Unresolved questions | ||
|
|
||
| N/A | ||
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.
s/unexpeditly/unexpectedly/