From 8fbb1724457697c264a2cca7376c8eca3ce1af64 Mon Sep 17 00:00:00 2001 From: Gaurav Munjal Date: Sat, 23 May 2020 14:11:28 -0400 Subject: [PATCH 1/9] Deprecate string based actions --- text/0000-deprecate-string-based-actions.md | 89 +++++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 text/0000-deprecate-string-based-actions.md diff --git a/text/0000-deprecate-string-based-actions.md b/text/0000-deprecate-string-based-actions.md new file mode 100644 index 0000000000..c1a75017fd --- /dev/null +++ b/text/0000-deprecate-string-based-actions.md @@ -0,0 +1,89 @@ +- Start Date: 2020-05-23 +- Relevant Team(s): Ember.js +- RFC PR: (after opening the RFC PR, update this with a link to it and update the file name) +- Tracking: (leave this empty) + +# Deprecate String Based Actions + +## Motivation + +> In Ember 1.13 we introduced [closure actions](https://github.com/emberjs/rfcs/blob/00ac2685c86f27d41547012903f485a4ef338d27/active/0000-improved-actions.md) have been recommended over string based actions. This officially deprecates string based actions in favor of closure actions, +which provide many benefits such as + +1. simpler passing through components +2. less need for an actions hash +3. ability to return a value +4. typing in TypeScript +5. compatible with Glimmer Components + +## Detailed Design + +We will deprecate the following: + +1. The `send` api. +2. The use of a string parameter to the `action` helper. + +## Transition Path + +Users will transition to closure actions. + +From: + +```js +export default Component.extend({ + someMethod() { + this.send('actionName'); + } + + actions: { + actionName() { + // do something + } + } +}); +``` + +To: + +```js +export default Component.extend({ + someMethod() { + this.actionName(); + } + + actionName() { + // do something + } +}); +``` + +From: + +```hbs + +``` + +To: + +```hbs + +``` + +## How We Teach This + +> We already teach closure actions exclusively in the guides. Other than a new +deprecation guide, no additional material should be necessary. + +## Drawbacks + +> Some older applications have thousands of string based actions throughout their codebases. +We should consider writing a codemod to ease the transition. + +## Alternatives + +> Closure actions are now the standard in Ember, so other alternatives have not been +seriously considered. + +## Unresolved questions + +> Can we also deprecate the use of the `actions` hash? From cd83cab2654257ad52dafb4498c92e9d3672a2f2 Mon Sep 17 00:00:00 2001 From: Gaurav Munjal Date: Sat, 23 May 2020 14:45:08 -0400 Subject: [PATCH 2/9] minor tweaks --- text/0000-deprecate-string-based-actions.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/text/0000-deprecate-string-based-actions.md b/text/0000-deprecate-string-based-actions.md index c1a75017fd..ec0728f381 100644 --- a/text/0000-deprecate-string-based-actions.md +++ b/text/0000-deprecate-string-based-actions.md @@ -33,7 +33,7 @@ From: export default Component.extend({ someMethod() { this.send('actionName'); - } + }, actions: { actionName() { @@ -49,7 +49,7 @@ To: export default Component.extend({ someMethod() { this.actionName(); - } + }, actionName() { // do something @@ -86,4 +86,4 @@ seriously considered. ## Unresolved questions -> Can we also deprecate the use of the `actions` hash? +> Can we also deprecate the use of the `actions` hash? Can we also deprecate using `this.actions`? From 3048b994f85767d4f4528baf3977358900aa2e89 Mon Sep 17 00:00:00 2001 From: Gaurav Munjal Date: Sat, 23 May 2020 15:14:24 -0400 Subject: [PATCH 3/9] mention action modifier --- text/0000-deprecate-string-based-actions.md | 1 + 1 file changed, 1 insertion(+) diff --git a/text/0000-deprecate-string-based-actions.md b/text/0000-deprecate-string-based-actions.md index ec0728f381..f74e43fefd 100644 --- a/text/0000-deprecate-string-based-actions.md +++ b/text/0000-deprecate-string-based-actions.md @@ -22,6 +22,7 @@ We will deprecate the following: 1. The `send` api. 2. The use of a string parameter to the `action` helper. +3. The use of a string parameter to the `action` modifier. ## Transition Path From 67728ad58beb71e5b11bf629f9965ed23d2855d7 Mon Sep 17 00:00:00 2001 From: Gaurav Munjal Date: Sat, 23 May 2020 20:26:58 -0400 Subject: [PATCH 4/9] more detail about send api --- text/0000-deprecate-string-based-actions.md | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/text/0000-deprecate-string-based-actions.md b/text/0000-deprecate-string-based-actions.md index f74e43fefd..7e30904e73 100644 --- a/text/0000-deprecate-string-based-actions.md +++ b/text/0000-deprecate-string-based-actions.md @@ -7,14 +7,14 @@ ## Motivation -> In Ember 1.13 we introduced [closure actions](https://github.com/emberjs/rfcs/blob/00ac2685c86f27d41547012903f485a4ef338d27/active/0000-improved-actions.md) have been recommended over string based actions. This officially deprecates string based actions in favor of closure actions, +> In Ember 1.13 we introduced [closure actions](https://github.com/emberjs/rfcs/blob/00ac2685c86f27d41547012903f485a4ef338d27/active/0000-improved-actions.md), which have been recommended over string based actions. This officially deprecates string based actions in favor of closure actions, which provide many benefits such as 1. simpler passing through components 2. less need for an actions hash 3. ability to return a value 4. typing in TypeScript -5. compatible with Glimmer Components +5. more in line with the Octane programming model ## Detailed Design @@ -24,6 +24,16 @@ We will deprecate the following: 2. The use of a string parameter to the `action` helper. 3. The use of a string parameter to the `action` modifier. +The `send` api may currently be used in components, controllers, and routes. +It is used to target the `actions` hash. In calling the `actions` hash of the +same object, `send` can be replaced by a function call once the method is +moved outside the `actions` hash. + +For cases where `send` is used to go from a controller to a route or from +a route to a parent route, no direct replacement for this "action bubbling" +behavior will be provided. +Instead, users are recommended to inject the router service. + ## Transition Path Users will transition to closure actions. From b6c3382a4f4607851160bc28ffda75ea7ab45dbc Mon Sep 17 00:00:00 2001 From: Gaurav Munjal Date: Tue, 26 May 2020 19:13:59 -0400 Subject: [PATCH 5/9] change transition path --- ...=> 0632-deprecate-string-based-actions.md} | 51 ++++++++++++++++--- 1 file changed, 44 insertions(+), 7 deletions(-) rename text/{0000-deprecate-string-based-actions.md => 0632-deprecate-string-based-actions.md} (68%) diff --git a/text/0000-deprecate-string-based-actions.md b/text/0632-deprecate-string-based-actions.md similarity index 68% rename from text/0000-deprecate-string-based-actions.md rename to text/0632-deprecate-string-based-actions.md index 7e30904e73..e89f02fe29 100644 --- a/text/0000-deprecate-string-based-actions.md +++ b/text/0632-deprecate-string-based-actions.md @@ -1,6 +1,6 @@ - Start Date: 2020-05-23 - Relevant Team(s): Ember.js -- RFC PR: (after opening the RFC PR, update this with a link to it and update the file name) +- RFC PR: https://github.com/emberjs/rfcs/pull/632 - Tracking: (leave this empty) # Deprecate String Based Actions @@ -38,7 +38,9 @@ Instead, users are recommended to inject the router service. Users will transition to closure actions. -From: +While the action helper and modifiers are still supported with non-string arguments, the refactoring recommendation is to switch to the on modifier and @action decorator directly. It is possible that the action helpers and modifiers will be deprecated in future RFCs. + +For example, take this button that sends the foo action with the default click event: ```js export default Component.extend({ @@ -54,7 +56,12 @@ export default Component.extend({ }); ``` -To: +```hbs + +``` + +Although this could be refactored to: + ```js export default Component.extend({ @@ -68,16 +75,45 @@ export default Component.extend({ }); ``` -From: +```hbs + +``` + +it is recommended that the code is refactored instead to: + +```js +export default Component.extend({ + someMethod() { + this.actionName(); + }, + + action(function actionName() { + // do something + }) +}); +``` ```hbs - + ``` -To: +so that it can eventually become: + +```js +export class extends Component { + someMethod() { + this.actionName(); + } + + @action + function actionName() { + // do something + } +} +``` ```hbs - + ``` ## How We Teach This @@ -98,3 +134,4 @@ seriously considered. ## Unresolved questions > Can we also deprecate the use of the `actions` hash? Can we also deprecate using `this.actions`? +Can the `target` property of `@ember/controller` and `@ember/component` be deprecated? From 13099a4c133aa4239a78dbd68c9538ff467ec9de Mon Sep 17 00:00:00 2001 From: Gaurav Munjal Date: Wed, 27 May 2020 12:48:27 -0400 Subject: [PATCH 6/9] Apply suggestions from code review Co-authored-by: Ilya Radchenko --- text/0632-deprecate-string-based-actions.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0632-deprecate-string-based-actions.md b/text/0632-deprecate-string-based-actions.md index e89f02fe29..a2235ec94d 100644 --- a/text/0632-deprecate-string-based-actions.md +++ b/text/0632-deprecate-string-based-actions.md @@ -87,7 +87,7 @@ export default Component.extend({ this.actionName(); }, - action(function actionName() { + actionName: action(function() { // do something }) }); @@ -106,7 +106,7 @@ export class extends Component { } @action - function actionName() { + actionName() { // do something } } From cde6e6f10c9e17c06b6477adc9d3811770514ab8 Mon Sep 17 00:00:00 2001 From: Gaurav Munjal Date: Thu, 28 May 2020 15:32:51 -0400 Subject: [PATCH 7/9] Apply suggestions from code review Co-authored-by: Mehul Kar --- text/0632-deprecate-string-based-actions.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/text/0632-deprecate-string-based-actions.md b/text/0632-deprecate-string-based-actions.md index a2235ec94d..2275a1dcaa 100644 --- a/text/0632-deprecate-string-based-actions.md +++ b/text/0632-deprecate-string-based-actions.md @@ -7,7 +7,7 @@ ## Motivation -> In Ember 1.13 we introduced [closure actions](https://github.com/emberjs/rfcs/blob/00ac2685c86f27d41547012903f485a4ef338d27/active/0000-improved-actions.md), which have been recommended over string based actions. This officially deprecates string based actions in favor of closure actions, +In Ember 1.13 we introduced [closure actions](https://github.com/emberjs/rfcs/blob/00ac2685c86f27d41547012903f485a4ef338d27/active/0000-improved-actions.md), which have been recommended over string based actions. This officially deprecates string based actions in favor of closure actions, which provide many benefits such as 1. simpler passing through components @@ -118,20 +118,20 @@ export class extends Component { ## How We Teach This -> We already teach closure actions exclusively in the guides. Other than a new +We already teach closure actions exclusively in the guides. Other than a new deprecation guide, no additional material should be necessary. ## Drawbacks -> Some older applications have thousands of string based actions throughout their codebases. +Some older applications have thousands of string based actions throughout their codebases. We should consider writing a codemod to ease the transition. ## Alternatives -> Closure actions are now the standard in Ember, so other alternatives have not been +Closure actions are now the standard in Ember, so other alternatives have not been seriously considered. ## Unresolved questions -> Can we also deprecate the use of the `actions` hash? Can we also deprecate using `this.actions`? +Can we also deprecate the use of the `actions` hash? Can we also deprecate using `this.actions`? Can the `target` property of `@ember/controller` and `@ember/component` be deprecated? From c9f87cd699a3b1da4e661262f782eda89beaa6d3 Mon Sep 17 00:00:00 2001 From: Gaurav Munjal Date: Fri, 29 May 2020 08:59:52 -0400 Subject: [PATCH 8/9] add send part to transition path --- text/0632-deprecate-string-based-actions.md | 64 ++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/text/0632-deprecate-string-based-actions.md b/text/0632-deprecate-string-based-actions.md index 2275a1dcaa..c65e109044 100644 --- a/text/0632-deprecate-string-based-actions.md +++ b/text/0632-deprecate-string-based-actions.md @@ -79,6 +79,7 @@ export default Component.extend({ ``` +Because the action modifier might be deprecated in the future, it is recommended that the code is refactored instead to: ```js @@ -97,7 +98,7 @@ export default Component.extend({ ``` -so that it can eventually become: +so that it can eventually become, once refactored to native classes: ```js export class extends Component { @@ -116,6 +117,67 @@ export class extends Component { ``` +For the send api, it is now generally possible to avoid string based actions in the actions +hash of any route, because the router service has all the methods that were once only +available in routes. If there are any route actions, it is recommended to move them to a +component, service, or a controller. + +For example, if you currently have a route action accessed by the `send` api like this: + +```js +// app/routes/some-route.js + +actions: { + myAction() { + this.transitionTo('some.other.route'); + } +} +``` + +and are calling it from using `send` in your controller like this: + +```js +// app/controllers/some-route.js +someMethod() { + this.send('myAction'); +} +``` + +You can move the action from the route to the controller: + +```js +// app/controllers/some-route.js +export default Controller.extend({ + router: service(), + + someMethod() { + this.myAction(); + } + + myAction: action(function() { + this.router.transitionTo('some.other.route'); + }) +}); +``` + +In native class syntax, this looks like: + +```js +// app/controllers/some-route.js +export class SomeRouteController extends Controller { + @service router; + + someMethod() { + this.myAction(); + } + + @action + myAction() { + this.router.transitionTo('some.other.route'); + } +} +``` + ## How We Teach This We already teach closure actions exclusively in the guides. Other than a new From b50d9c60b6fb0b54f2db23ded31bf9782e1e6963 Mon Sep 17 00:00:00 2001 From: Gaurav Munjal Date: Fri, 29 May 2020 13:05:41 -0400 Subject: [PATCH 9/9] transition path for route-action --- text/0632-deprecate-string-based-actions.md | 42 ++++++++++++++++++++- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/text/0632-deprecate-string-based-actions.md b/text/0632-deprecate-string-based-actions.md index c65e109044..b2fb0ea9fd 100644 --- a/text/0632-deprecate-string-based-actions.md +++ b/text/0632-deprecate-string-based-actions.md @@ -101,7 +101,7 @@ export default Component.extend({ so that it can eventually become, once refactored to native classes: ```js -export class extends Component { +export default class extends Component { someMethod() { this.actionName(); } @@ -164,7 +164,7 @@ In native class syntax, this looks like: ```js // app/controllers/some-route.js -export class SomeRouteController extends Controller { +export default class SomeRouteController extends Controller { @service router; someMethod() { @@ -178,6 +178,44 @@ export class SomeRouteController extends Controller { } ``` +If you are using `ember-route-action-helper`, you can instead +move your action from the route to the controller. + +From: + +```js +// app/routes/some-route.js +export default Route.extend({ + actions: { + someRouteAction() { + // do something + } + } +} +``` + +```hbs + + +``` + +To: + +```js +// app/routes/some-controller.js +export default class SomeController extends Controller { + @action + someAction() { + // so something + } +} +``` + +```hbs + + +``` + ## How We Teach This We already teach closure actions exclusively in the guides. Other than a new