Skip to content
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 string based actions #632

Closed
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
237 changes: 237 additions & 0 deletions text/0632-deprecate-string-based-actions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,237 @@
- Start Date: 2020-05-23
- Relevant Team(s): Ember.js
- RFC PR: https://github.com/emberjs/rfcs/pull/632
- 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), 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. more in line with the Octane programming model

## Detailed Design

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.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could elaborate on this a bit?

Our app has probably ~30% of all actions in routes. Right now this is treated more like a side-effect, but for us the biggest change here would probably be what to do with all our route actions? Or perhaps I've misunderstood something?

Since things like routable components (or something to that effect) isn't available, there isn't really a clear migration path? (that I'm aware of)

Also, if all the docs are already teaching the closure actions, maybe this could be put on hold until a replacement for "actions in routes" is available?

Overall still in favor of the change, but the effect this would have on action bubbling could be expanded a bit.

Instead, users are recommended to inject the router service.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain this part? How does the router service remove the need to use "action bubbling"? As someone who has lots of instances of using send in controllers/routes it would be good to have a concrete example in the Transition Path section too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, there is no longer a need to use actions in a route because the router service exposes all the methods that used to be available on routes.

It may be that you have a good use case for keeping your actions in a route. If you do, let me know.

This is the idea: if you have an action in your route that would transition to a different route, move it to your component or any service and just call the routerService's transitionTo method.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that clarifies things a bit. I wonder if you could include some of this text in the rfc because that sentence is a bit vague. I'd also still recommend having an example in the Transition Path section.


## Transition Path

Users will transition to closure actions.

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({
someMethod() {
this.send('actionName');
},

actions: {
actionName() {
// do something
}
}
});
```

```hbs
<button {{action 'actionName'}}>Action Label</button>
```

Although this could be refactored to:


```js
export default Component.extend({
someMethod() {
this.actionName();
},

actionName() {
// do something
}
});
```

```hbs
<button {{action this.actionName}}>Action Label</button>
```

Because the action modifier might be deprecated in the future,
it is recommended that the code is refactored instead to:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it is recommended that the code is refactored instead to:
it is recommended that the code is refactored instead to, because the above example looses its `this` context:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The action modifier does bind the this context correctly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was about the bare function above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if there is an action modifier in the template, the function can be bare.

Copy link
Contributor

@knownasilya knownasilya May 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly, once you do something like @someaction={{this.actionName}} it breaks down. Which seems like a valid thing to want to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. However, you can fix it as @someaction={{action this.actionName}}.


```js
export default Component.extend({
someMethod() {
this.actionName();
},

actionName: action(function() {
// do something
})
});
```

```hbs
<button {{on "click" this.actionName}}>Action Label</button>
```

so that it can eventually become, once refactored to native classes:

```js
export default class extends Component {
someMethod() {
this.actionName();
}

@action
actionName() {
// do something
}
}
```

```hbs
<button {{on "click" this.actionName}}>Action Label</button>
```

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 default class SomeRouteController extends Controller {
@service router;

someMethod() {
this.myAction();
}

@action
myAction() {
this.router.transitionTo('some.other.route');
}
}
```

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
<!-- app/templates/some-route.hbs -->
<SomeComponent myAction={{route-action 'someRouteAction'}} />
```

To:

```js
// app/routes/some-controller.js
export default class SomeController extends Controller {
@action
someAction() {
// so something
}
}
```

```hbs
<!-- app/templates/some-route.hbs -->
<SomeComponent myAction={{someAction}} />
```

## 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? Can we also deprecate using `this.actions`?
Can the `target` property of `@ember/controller` and `@ember/component` be deprecated?