From 3801c1984ad2dfa47dc80d5bbc4ae27006b6c777 Mon Sep 17 00:00:00 2001 From: Miguel Camba Date: Fri, 31 Jul 2015 09:55:59 +0100 Subject: [PATCH 1/5] RFC about improve CPs ergonomics by changing caching strategy --- text/more-friendly-cp-caching.md | 125 +++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+) create mode 100644 text/more-friendly-cp-caching.md diff --git a/text/more-friendly-cp-caching.md b/text/more-friendly-cp-caching.md new file mode 100644 index 0000000000..ae5e2f6085 --- /dev/null +++ b/text/more-friendly-cp-caching.md @@ -0,0 +1,125 @@ +- Start Date: 2015-07-31 +- RFC PR: (leave this empty) +- Ember Issue: (leave this empty) + +# Summary + +At the moment, the setter function in computed properties caches the return value of the function, +even when the return value is undefined. While undefined is a perfectly valid value, cases where +you really want to cache undefined are very rare, and most of the time is just the user forgetting +to return from the function. + +I think there is a better (or even completely) retrocompatible way to improve user experience +and be closer to regular javascript. + +# Motivation + +I've found that computed properties with getters and setters keep bitting newcomers because they +don't behave like regular JS getters and setters, and I think they can be more user friendly and +also more aligned with regular javascript. + +This is the kind of code that keeps confusing users: + +```js +height: 100, +goldenRatioWidth: computed('height', { + get() { + return this.get('height') * 1.618; + }, + set(_, value) { + this.set('height', value / 1.618); + } +}) +``` + +I've explained myself half a docen times in the last month to confused programmers why this +snippet doesn't work as expected, and it's easy to see why they are confused. The equivalent +code with regular getters and setters whould look like this: + +```js +heigth: 100, +get goldenRatioWidth() { + return this.height * 1.618; +}, +set goldenRatioWidth(value) { + this.height = value / 1.618; +} +``` + +No return, but the code works as expected because there is no caching, and the next time the +property accessed the getter will be invoked and the expected result will be computed. + +I think that computed properties should be as close to this behavior as they possibly can, +specially when code with decorators will look like this: + +```js +@computed('height') +get goldenRatioWidth() { + return this.height * 1.618; +}, +set goldenRatioWidth(value) { + this.height = value / 1.618; +} +``` + +In this snipped above I'd expect the code to behave the same with and without the decorator except +for the "caching with declarative expiration policy" that the decorator adds. + +# Detailed design + +The reason why people gest confused is that most people doesn't expect setter to cache. +Although your getter and setter can do any crazy stuff, in most sanely defined computed properties +developers look for "idempotence" (not sure if it's the better term). When you invoke +`set('myProp', 200)` they expect the next time they get the property with `get('myProp')` to +get the same value they just set. + +When someone implements a getter in a computed property is because setting state in this +computed property: + +A) Has to set state in another property, like the example above. Tipically that other property +is one of the dependent keys. + +B) Has to trigger some callback-like logic. Tipically this is way of replacing an observer. + +In computed properties for case A, after setting the state in a dependent key the users, used to +the way ES5 getters/setters work, think that they're done, because the next time the property is +accessed the getter will be invoked and the expected result will come out. + +And it doesn't because getter cache `undefined` as return value. I dare to affirm that is almost never +the indended behaviour. + +One radical solution would be not cache at all in setters (getters would cache as usual). +This is a rather aggresive change, although I think that "idemponent" computed properties +(the big mayority) the getter would return the exact same value and nobody would notice the +difference, but in a few uses cases where users use the cache to just store a calculation that +would be a pain, forcing users to store that same value in a `_private` key by example. +On the bright side, this is as close to ES5 getters/setters as you can posibly be. + +A mostly retrocompatible option would be to **not cache undefined** from the setters. If the user +doesn't returns from a setter, nothing is memoized, the property is just marked as stale +and the next access will invoke the getter. Getters would continue to cache undefined as they do now. +If a user returns a value, it works as usual. However `return undefined;` won't cache the value. +That is the only non retrocompatible change. Computed properties that exist in the wild right now +will all have a explicit return and won't be affected. + +@stefanpenner in slack suggested and improvement to the second approach that would make this +completely retrocompatible in exchange for a bit of computational cost. When the setter returns +`undefined` the can do `setter.toString().indexOf('return')` to determine if it was +intended or not. Not sure if the case of caching `undefined` from a getter is common enough to +justify this, and how unsafe it's to do that (in can be edge cases with the detection of explicit return) + +# Drawbacks + +The first approach is not retrocompatible and makes some use cases less straightforward. + +The second approach is mostly retrocompatible, but not completely. + +# Alternatives + +Leave the bahaviour as it is right now and just educate users. This is what I've been doing but people +stills get confused, so clearly we have to try harder. + +# Unresolved questions + +Is it possible to detect if a function does explicit return in "compile time"? That could make +possible a completely retrocompatible solution with no performance cost. From ad2240baaee410db146c63e980af6cccfaf86a54 Mon Sep 17 00:00:00 2001 From: Miguel Camba Date: Fri, 31 Jul 2015 10:04:27 +0100 Subject: [PATCH 2/5] Fix typo --- text/more-friendly-cp-caching.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/more-friendly-cp-caching.md b/text/more-friendly-cp-caching.md index ae5e2f6085..e6c4053c1f 100644 --- a/text/more-friendly-cp-caching.md +++ b/text/more-friendly-cp-caching.md @@ -85,7 +85,7 @@ In computed properties for case A, after setting the state in a dependent key th the way ES5 getters/setters work, think that they're done, because the next time the property is accessed the getter will be invoked and the expected result will come out. -And it doesn't because getter cache `undefined` as return value. I dare to affirm that is almost never +And it doesn't because setters cache `undefined` as return value. I dare to affirm that is almost never the indended behaviour. One radical solution would be not cache at all in setters (getters would cache as usual). From de8e450a99dd3e496c016049e46cb01b8ede1e25 Mon Sep 17 00:00:00 2001 From: Jamie White Date: Fri, 31 Jul 2015 17:05:14 +0100 Subject: [PATCH 3/5] Correct a few typos in more-friendly-cp-caching.md --- text/more-friendly-cp-caching.md | 40 ++++++++++++++++---------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/text/more-friendly-cp-caching.md b/text/more-friendly-cp-caching.md index e6c4053c1f..52aac8af50 100644 --- a/text/more-friendly-cp-caching.md +++ b/text/more-friendly-cp-caching.md @@ -6,7 +6,7 @@ At the moment, the setter function in computed properties caches the return value of the function, even when the return value is undefined. While undefined is a perfectly valid value, cases where -you really want to cache undefined are very rare, and most of the time is just the user forgetting +you really want to cache undefined are very rare, and most of the time it's just the user forgetting to return from the function. I think there is a better (or even completely) retrocompatible way to improve user experience @@ -14,7 +14,7 @@ and be closer to regular javascript. # Motivation -I've found that computed properties with getters and setters keep bitting newcomers because they +I've found that computed properties with getters and setters keep biting newcomers because they don't behave like regular JS getters and setters, and I think they can be more user friendly and also more aligned with regular javascript. @@ -32,12 +32,12 @@ goldenRatioWidth: computed('height', { }) ``` -I've explained myself half a docen times in the last month to confused programmers why this +I've explained myself half a dozen times in the last month to confused programmers why this snippet doesn't work as expected, and it's easy to see why they are confused. The equivalent -code with regular getters and setters whould look like this: +code with regular getters and setters would look like this: ```js -heigth: 100, +height: 100, get goldenRatioWidth() { return this.height * 1.618; }, @@ -47,10 +47,10 @@ set goldenRatioWidth(value) { ``` No return, but the code works as expected because there is no caching, and the next time the -property accessed the getter will be invoked and the expected result will be computed. +property is accessed the getter will be invoked and the expected result will be computed. I think that computed properties should be as close to this behavior as they possibly can, -specially when code with decorators will look like this: +especially when code with decorators will look like this: ```js @computed('height') @@ -62,24 +62,24 @@ set goldenRatioWidth(value) { } ``` -In this snipped above I'd expect the code to behave the same with and without the decorator except +In the snippet above I'd expect the code to behave the same with and without the decorator except for the "caching with declarative expiration policy" that the decorator adds. # Detailed design -The reason why people gest confused is that most people doesn't expect setter to cache. +The reason why people get confused is that most people doesn't expect the setter to cache. Although your getter and setter can do any crazy stuff, in most sanely defined computed properties developers look for "idempotence" (not sure if it's the better term). When you invoke `set('myProp', 200)` they expect the next time they get the property with `get('myProp')` to get the same value they just set. -When someone implements a getter in a computed property is because setting state in this +When someone implements a getter in a computed property, it's because setting state in this computed property: -A) Has to set state in another property, like the example above. Tipically that other property +A) Has to set state in another property, like the example above. Typically that other property is one of the dependent keys. -B) Has to trigger some callback-like logic. Tipically this is way of replacing an observer. +B) Has to trigger some callback-like logic. Typically this is way of replacing an observer. In computed properties for case A, after setting the state in a dependent key the users, used to the way ES5 getters/setters work, think that they're done, because the next time the property is @@ -88,25 +88,25 @@ accessed the getter will be invoked and the expected result will come out. And it doesn't because setters cache `undefined` as return value. I dare to affirm that is almost never the indended behaviour. -One radical solution would be not cache at all in setters (getters would cache as usual). -This is a rather aggresive change, although I think that "idemponent" computed properties +One radical solution would be not to cache at all in setters (getters would cache as usual). +This is a rather aggressive change, although I think that "idemponent" computed properties (the big mayority) the getter would return the exact same value and nobody would notice the -difference, but in a few uses cases where users use the cache to just store a calculation that +difference, but in a few use cases where users use the cache to just store a calculation that would be a pain, forcing users to store that same value in a `_private` key by example. On the bright side, this is as close to ES5 getters/setters as you can posibly be. A mostly retrocompatible option would be to **not cache undefined** from the setters. If the user -doesn't returns from a setter, nothing is memoized, the property is just marked as stale +doesn't return from a setter, nothing is memoized, the property is just marked as stale and the next access will invoke the getter. Getters would continue to cache undefined as they do now. If a user returns a value, it works as usual. However `return undefined;` won't cache the value. That is the only non retrocompatible change. Computed properties that exist in the wild right now will all have a explicit return and won't be affected. -@stefanpenner in slack suggested and improvement to the second approach that would make this +@stefanpenner in slack suggested an improvement to the second approach that would make this completely retrocompatible in exchange for a bit of computational cost. When the setter returns `undefined` the can do `setter.toString().indexOf('return')` to determine if it was intended or not. Not sure if the case of caching `undefined` from a getter is common enough to -justify this, and how unsafe it's to do that (in can be edge cases with the detection of explicit return) +justify this, and how unsafe it is to do that (it can be edge cases with the detection of explicit return). # Drawbacks @@ -117,9 +117,9 @@ The second approach is mostly retrocompatible, but not completely. # Alternatives Leave the bahaviour as it is right now and just educate users. This is what I've been doing but people -stills get confused, so clearly we have to try harder. +still get confused, so clearly we have to try harder. # Unresolved questions -Is it possible to detect if a function does explicit return in "compile time"? That could make +Is it possible to detect if a function does explicitly return in "compile time"? That could make possible a completely retrocompatible solution with no performance cost. From 07dc952c5f7868553d4929bf33e12cf9d669b816 Mon Sep 17 00:00:00 2001 From: Miguel Camba Date: Fri, 29 Jan 2016 13:15:39 +0000 Subject: [PATCH 4/5] Fix typo --- text/more-friendly-cp-caching.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/more-friendly-cp-caching.md b/text/more-friendly-cp-caching.md index 52aac8af50..51795951fc 100644 --- a/text/more-friendly-cp-caching.md +++ b/text/more-friendly-cp-caching.md @@ -73,7 +73,7 @@ developers look for "idempotence" (not sure if it's the better term). When you i `set('myProp', 200)` they expect the next time they get the property with `get('myProp')` to get the same value they just set. -When someone implements a getter in a computed property, it's because setting state in this +When someone implements a setter in a computed property, it's because setting state in this computed property: A) Has to set state in another property, like the example above. Typically that other property From 1990cda35e467a524d42af18bca05abec2e84686 Mon Sep 17 00:00:00 2001 From: Miguel Camba Date: Fri, 29 Jan 2016 13:27:45 +0000 Subject: [PATCH 5/5] Add 4th plan. --- text/more-friendly-cp-caching.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/text/more-friendly-cp-caching.md b/text/more-friendly-cp-caching.md index 51795951fc..f1c30ed09c 100644 --- a/text/more-friendly-cp-caching.md +++ b/text/more-friendly-cp-caching.md @@ -108,17 +108,29 @@ completely retrocompatible in exchange for a bit of computational cost. When the intended or not. Not sure if the case of caching `undefined` from a getter is common enough to justify this, and how unsafe it is to do that (it can be edge cases with the detection of explicit return). +Another 4th option that might enable this is wait for decorators to land. If there is enough reasons to take +this approach, the `@computed('foo', 'bar') + & ` decorator can have slightly different (better) +semantics than `computed('foo', 'bar', { get() { }, set(_, v){ } })`. It's new syntax after all, so it's not +technically a breaking change. + # Drawbacks The first approach is not retrocompatible and makes some use cases less straightforward. The second approach is mostly retrocompatible, but not completely. +The third approach is is probably completely retrocompatible, at the expense of being computationally more expensive. + +The forth approach introduced new semantics when using the `@computed` decorator, so it's not technically a breaking +change, but both old and new computed properties have to coexist until Ember 3.0 and it has to be very well communicated +, documented, twitted and blogged so people understand that `@computed` and `computed(fn)` are not entirely equivalent. + # Alternatives Leave the bahaviour as it is right now and just educate users. This is what I've been doing but people still get confused, so clearly we have to try harder. + # Unresolved questions Is it possible to detect if a function does explicitly return in "compile time"? That could make