From e02d3ce6cfeed768e992c54bff31b17ebc7b18d5 Mon Sep 17 00:00:00 2001 From: Christopher Garrett Date: Fri, 31 Aug 2018 11:14:45 -0700 Subject: [PATCH 1/5] deprecate computed clobberability --- .../0000-deprecate-computed-clobberability.md | 146 ++++++++++++++++++ 1 file changed, 146 insertions(+) create mode 100644 text/0000-deprecate-computed-clobberability.md diff --git a/text/0000-deprecate-computed-clobberability.md b/text/0000-deprecate-computed-clobberability.md new file mode 100644 index 0000000000..e71fae5525 --- /dev/null +++ b/text/0000-deprecate-computed-clobberability.md @@ -0,0 +1,146 @@ +- Start Date: 2018-08-30 +- RFC PR: (leave this empty) +- Ember Issue: (leave this empty) + +# Summary + +Deprecate computed clobberability and `computed().readOnly()` in favor of +read-only computeds as the default. + +# Motivation + +Computed properties have existed in Ember long before class syntax and native +accessors (getters and setters) were readily available, and as such they have a +few notable behavioral differences. As we move toward adopting native class +syntax and using a decorator-based form of computeds, it makes sense to +reconcile these differences so that users can expect them to work the same as +their native counterparts. + +The main and most notable difference this RFC seeks to deprecate is computed +clobberability. There are some other notable differences, including the caching +behavior of the `return` value of setter functions, which may be addressed in +future RFCs. + +## Clobberability + +When defining a native getter without a setter, attempting to set the value will +throw a hard error (in strict mode): + +```js +function makeFoo() { + 'use strict'; + + class Foo { + get bar() { + return this._value; + } +} + +let foo = new Foo(); + +foo.bar; // undefined +foo.bar = 'baz'; // throws an error in strict mode +} +``` + +By constrast, computed properties without setters will be "clobbered" when they +are set, meaning the computed property is removed from the object and replaced +with the set value: + +```js +const Foo = EmberObject.extend({ + bar: computed('_value', { + get() { + return this._value; + }, + }), +}); + +let foo = Foo.create(); + +foo.bar; // undefined +foo.set('bar', 'baz'); // Overwrites the getter +foo.bar; // 'baz' +foo.set('_value', 123); +foo.bar; // 'baz' +``` + +This behavior is confusing to newcomers, and oftentimes unexpected. Common best +practice is to opt-out of it by declaring the property as `readOnly`, which +prevents this clobberability. + +# Transition Path + +This RFC proposes that `readOnly` properties become the default, and that in +order to clobber users must opt in by defining their own setters: + +```js +class Foo { + get bar() { + if (this._bar) { + return this._bar; + } + + return this._value + } + + set bar(value) { + this._bar = value + } +} +``` + +## Macros + +Most computed macros are clobberable by default, the exception being `readOnly`. +This RFC proposes that all computed macros with the exception of `reads` would +become read only by default. The purpose of `reads` is to _be_ clobberable, so +its behavior would remain the same. + +## Decorator Interop + +It may be somewhat cumbersome to write clobbering functionality or add proxy +properties when clobbering is needed. In an ideal world, computed properties +would modify accessors transparently so that they could be composed with other +decorators, such as an `@clobberable` decorator: + +```js +class Foo { + @clobberable + @computed('_value') + get bar() { + return this._value; + } + + @clobberable + @and('baz', 'qux') + quux; +} +``` + +Currently this is not possible as computed properties store their getter/setter +functions elsewhere and replace them with a proxy getter and the mandatory +setter assertion, respectively. In the long term, making computeds more +transparent in this way would be ideal, but it is out of scope for this RFC. + +# How We Teach This + +In general, we can teach that computed properties are essentially cached native +getters/setters (with a few more bells and whistles). Once we have official +decorators in the framework, we can make this connection even more solid. + +We should add notes on clobberability, and we should scrub the guides of any +examples that make use of clobbering directly and indirectly via `.readOnly()`. + +# Drawbacks + +Clobbering is not a completely uncommonly used feature, and developers who have +become used to it may feel like it makes their code more complicated, especially +without any easy way to opt back in. + +# Alternatives + +We could convert `.readOnly()` into `.clobberable()`, forcing users to opt-in +to clobbering. Given the long timeline of this deprecation, it would likely be +better to work on making getters/setters transparent to decoration, and provide +a `@clobberable` decorator either in Ember or as an independent package. From e61cddfd1ef97c6ce8c5cf8e7a4fb0d7bac53fe5 Mon Sep 17 00:00:00 2001 From: Christopher Garrett Date: Fri, 31 Aug 2018 12:12:02 -0700 Subject: [PATCH 2/5] clobber -> override --- .../0000-deprecate-computed-clobberability.md | 42 +++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/text/0000-deprecate-computed-clobberability.md b/text/0000-deprecate-computed-clobberability.md index e71fae5525..e26116bc13 100644 --- a/text/0000-deprecate-computed-clobberability.md +++ b/text/0000-deprecate-computed-clobberability.md @@ -4,7 +4,7 @@ # Summary -Deprecate computed clobberability and `computed().readOnly()` in favor of +Deprecate computed overridability and `computed().readOnly()` in favor of read-only computeds as the default. # Motivation @@ -17,11 +17,11 @@ reconcile these differences so that users can expect them to work the same as their native counterparts. The main and most notable difference this RFC seeks to deprecate is computed -clobberability. There are some other notable differences, including the caching -behavior of the `return` value of setter functions, which may be addressed in -future RFCs. +overridability (colloquially known as "clobbering"). There are some other +notable differences, including the caching behavior of the `return` value of +setter functions, which may be addressed in future RFCs. -## Clobberability +## Overridability When defining a native getter without a setter, attempting to set the value will throw a hard error (in strict mode): @@ -43,7 +43,7 @@ foo.bar = 'baz'; // throws an error in strict mode } ``` -By constrast, computed properties without setters will be "clobbered" when they +By constrast, computed properties without setters will be overridden when they are set, meaning the computed property is removed from the object and replaced with the set value: @@ -67,12 +67,12 @@ foo.bar; // 'baz' This behavior is confusing to newcomers, and oftentimes unexpected. Common best practice is to opt-out of it by declaring the property as `readOnly`, which -prevents this clobberability. +prevents this overridability. # Transition Path This RFC proposes that `readOnly` properties become the default, and that in -order to clobber users must opt in by defining their own setters: +order to override users must opt in by defining their own setters: ```js class Foo { @@ -92,27 +92,27 @@ class Foo { ## Macros -Most computed macros are clobberable by default, the exception being `readOnly`. +Most computed macros are overridable by default, the exception being `readOnly`. This RFC proposes that all computed macros with the exception of `reads` would -become read only by default. The purpose of `reads` is to _be_ clobberable, so +become read only by default. The purpose of `reads` is to _be_ overridable, so its behavior would remain the same. ## Decorator Interop -It may be somewhat cumbersome to write clobbering functionality or add proxy -properties when clobbering is needed. In an ideal world, computed properties +It may be somewhat cumbersome to write overriding functionality or add proxy +properties when overriding is needed. In an ideal world, computed properties would modify accessors transparently so that they could be composed with other -decorators, such as an `@clobberable` decorator: +decorators, such as an `@overridable` decorator: ```js class Foo { - @clobberable + @overridable @computed('_value') get bar() { return this._value; } - @clobberable + @overridable @and('baz', 'qux') quux; } @@ -129,18 +129,18 @@ In general, we can teach that computed properties are essentially cached native getters/setters (with a few more bells and whistles). Once we have official decorators in the framework, we can make this connection even more solid. -We should add notes on clobberability, and we should scrub the guides of any -examples that make use of clobbering directly and indirectly via `.readOnly()`. +We should add notes on overridability, and we should scrub the guides of any +examples that make use of overriding directly and indirectly via `.readOnly()`. # Drawbacks -Clobbering is not a completely uncommonly used feature, and developers who have +Overriding is not a completely uncommonly used feature, and developers who have become used to it may feel like it makes their code more complicated, especially without any easy way to opt back in. # Alternatives -We could convert `.readOnly()` into `.clobberable()`, forcing users to opt-in -to clobbering. Given the long timeline of this deprecation, it would likely be +We could convert `.readOnly()` into `.overridable()`, forcing users to opt-in +to overriding. Given the long timeline of this deprecation, it would likely be better to work on making getters/setters transparent to decoration, and provide -a `@clobberable` decorator either in Ember or as an independent package. +a `@overridable` decorator either in Ember or as an independent package. From 24f90a63c2f0ea25e0afea3701988484f6463636 Mon Sep 17 00:00:00 2001 From: Christopher Garrett Date: Tue, 11 Sep 2018 16:30:15 -0700 Subject: [PATCH 3/5] update deprecation timeline --- text/0000-deprecate-computed-clobberability.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/text/0000-deprecate-computed-clobberability.md b/text/0000-deprecate-computed-clobberability.md index e26116bc13..b674fa8bf3 100644 --- a/text/0000-deprecate-computed-clobberability.md +++ b/text/0000-deprecate-computed-clobberability.md @@ -123,6 +123,18 @@ functions elsewhere and replace them with a proxy getter and the mandatory setter assertion, respectively. In the long term, making computeds more transparent in this way would be ideal, but it is out of scope for this RFC. +## Deprecation Timeline + +This change will be a breaking change, which means we will not be able to change +the behavior of `computed` until Ember v4.0.0. Until then, we should provide two +deprecation warnings: + +1. When users invoke the default override-setter behavior +2. When users call `.readOnly()` on a computed property + +The warnings should explain the deprecation, and recommend that users do not +rely on setter behavior or opting-in to read only behavior. + # How We Teach This In general, we can teach that computed properties are essentially cached native From ff6508789ceb094529166fd2a5c0680eed9c2cc2 Mon Sep 17 00:00:00 2001 From: Christopher Garrett Date: Thu, 13 Sep 2018 11:18:44 -0700 Subject: [PATCH 4/5] extend deprecation timeline, fix indentation --- .../0000-deprecate-computed-clobberability.md | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/text/0000-deprecate-computed-clobberability.md b/text/0000-deprecate-computed-clobberability.md index b674fa8bf3..12b51a4209 100644 --- a/text/0000-deprecate-computed-clobberability.md +++ b/text/0000-deprecate-computed-clobberability.md @@ -31,15 +31,15 @@ function makeFoo() { 'use strict'; class Foo { - get bar() { - return this._value; + get bar() { + return this._value; + } } -} -let foo = new Foo(); + let foo = new Foo(); -foo.bar; // undefined -foo.bar = 'baz'; // throws an error in strict mode + foo.bar; // undefined + foo.bar = 'baz'; // throws an error in strict mode } ``` @@ -126,11 +126,14 @@ transparent in this way would be ideal, but it is out of scope for this RFC. ## Deprecation Timeline This change will be a breaking change, which means we will not be able to change -the behavior of `computed` until Ember v4.0.0. Until then, we should provide two -deprecation warnings: - -1. When users invoke the default override-setter behavior -2. When users call `.readOnly()` on a computed property +the behavior of `computed` until Ember v4.0.0. Additionally, users will likely +want to continue using `.readOnly()` up until overriding has been fully removed +to ensure they are using properties safely. With that in mind, the ordering of +events should be: + +1. Deprecate the default override-setter behavior immediately, remove it in +Ember v4 +2. Deprecate the `.readOnly()` modifier in Ember v4, remove it in v5 The warnings should explain the deprecation, and recommend that users do not rely on setter behavior or opting-in to read only behavior. From 3f94ebf1fd341eb81f721189940af675f7448afd Mon Sep 17 00:00:00 2001 From: Christopher Garrett Date: Tue, 18 Sep 2018 14:13:03 -0700 Subject: [PATCH 5/5] clarify deprecation timeline --- .../0000-deprecate-computed-clobberability.md | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/text/0000-deprecate-computed-clobberability.md b/text/0000-deprecate-computed-clobberability.md index 12b51a4209..4fc17f5ae0 100644 --- a/text/0000-deprecate-computed-clobberability.md +++ b/text/0000-deprecate-computed-clobberability.md @@ -131,9 +131,24 @@ want to continue using `.readOnly()` up until overriding has been fully removed to ensure they are using properties safely. With that in mind, the ordering of events should be: -1. Deprecate the default override-setter behavior immediately, remove it in -Ember v4 -2. Deprecate the `.readOnly()` modifier in Ember v4, remove it in v5 +1. Ember v3 + * Deprecate the default override-setter behavior immediately. This means that + a deprecation warning will be thrown if a user attempts to set a + non-`readOnly` property which does not have a setter. User's will still be + able to declare a property is `readOnly` without a deprecation warning. + * Add optional feature to change the deprecation to an assertion after the + deprecation has been released, and to show a deprecation when using + the `.readOnly()` modifier. + * After the deprecation and optional feature have been available for a + reasonable amount of time, enable the optional feature by default in new + apps and addons. The main reason we want to delay this is to give _addons_ + a chance to address deprecations, since enabling this feature will affect + both apps and the addons they consume. +2. Ember v4 + * Remove the override-setter entirely, making non-overrideable properties the + default. + * Make the `readOnly` modifier a no-op, and show a deprecation warning when it + is used. The warnings should explain the deprecation, and recommend that users do not rely on setter behavior or opting-in to read only behavior.