Deprecate string based actions #632

Gaurav0 · 2020-05-23T18:12:11Z

For issue #629

text/0000-deprecate-string-based-actions.md

rwjblue · 2020-05-23T18:23:01Z

text/0000-deprecate-string-based-actions.md

+4. typing in TypeScript
+5. compatible with Glimmer Components
+
+## Detailed Design


This is missing quite a bit of info I think.

Which framework classes are affected?

What are the replacement values for each of the various use cases for .send?

Shouldn't it also deprecate directly accessing this.actions (this is a thing some folks have done, and the @action decorator still populates this actions hash)?

Does it deprecate specifying an actions hash?

What is the exact prose that will be included in the deprecation guide entry?

Shouldn't it also deprecate directly accessing this.actions (this is a thing some folks have done, and the @action decorator still populates this actions hash)?

Added to unresolved questions for the moment.

Does it deprecate specifying an actions hash?

Added to unresolved questions for the moment.

Which framework classes are affected?

Added.

What are the replacement values for each of the various use cases for .send?

Partially added.

What is the exact prose that will be included in the deprecation guide entry?

Help wanted here.

rwjblue · 2020-05-23T18:26:15Z

text/0000-deprecate-string-based-actions.md

+        this.actionName();
+    }
+
+    actionName() {


This should use @action

@action will not work with .extend(; i.e. it would require a native class.

I'm using @action here to show what I mean, you can easily use that decorator in a classic Ember.Object.extend style class (just like computed):

export default Component.extend({ actionName: action(function() { }), })

I'm doubtful we should be recommending this. This syntax is not shown in any guides, pre- or post-Octane, and will be unfamiliar.

It was recommended in the 3.11 release blog post https://blog.emberjs.com/2019/07/15/ember-3-11-released.html, we used this syntax pretty extensively while migrating!

If there is a gap in the guides (though I’m not sure there is) then we should fix it. That is something this RFC should work to address.

That fact does not change the direction we should advise folks to migrate to. Migrating as proposed in this RFC at the moment would be a non-trivial waste of time, and ultimately (IMO) not worth moving forward RFC wise.

Ok. I'm putting this in. Where in the guides can we discuss this?

Not 100% sure, maybe a "classic interop" thing in the advanced topics?

@emberjs/learning-team-managers - Thoughts?

text/0000-deprecate-string-based-actions.md

rwjblue · 2020-05-23T18:29:19Z

text/0000-deprecate-string-based-actions.md

+## Drawbacks
+
+> Some older applications have thousands of string based actions throughout their codebases.
+We should consider writing a codemod to ease the transition.


This RFC's detailed design or transition path should include the details that the codemod should use. What would the codemod's heuristics be? How would it select what to update? How would it determine what to update to?

Do you have an example RFC that does this?

mehulkar · 2020-05-24T22:58:23Z

text/0000-deprecate-string-based-actions.md

+
+## Unresolved questions
+
+> Can we also deprecate the use of the `actions` hash? Can we also deprecate using `this.actions`?


I think the difficult here is going to be less about "deprecating" this.actions, it'll be more about figuring out how to teach people that the actions key on a class no longer holds any special significance.

I think I agree. Would you be willing to PR this PR and help out? Probably belongs in "How we teach this" section.

text/0000-deprecate-string-based-actions.md

knownasilya

Some small changes

knownasilya · 2020-05-27T15:56:19Z

text/0632-deprecate-string-based-actions.md

+<button {{action this.actionName}}>Action Label</button>
+```
+
+it is recommended that the code is refactored instead to:


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:

The action modifier does bind the this context correctly.

That was about the bare function above.

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

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.

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

text/0632-deprecate-string-based-actions.md

Co-authored-by: Ilya Radchenko <knownasilya@gmail.com>

mehulkar

My middle name is Captain Pedantic!

text/0632-deprecate-string-based-actions.md

Co-authored-by: Mehul Kar <mehul.kar@gmail.com>

patocallaghan · 2020-05-28T21:54:03Z

text/0632-deprecate-string-based-actions.md

+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.


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.

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.

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.

luxzeitlos · 2020-05-29T17:25:25Z

I really like this, but there is one very important use case that we need to cover before we can do this. And this is the Route#refresh method.

While you proposed a new refresh method on the RouterService, which is good, it does not cover this use-case! There must be a way to refresh the current Route from the template/controller.

Gaurav0 · 2020-05-29T18:09:35Z

I really like this, but there is one very important use case that we need to cover before we can do this. And this is the Route#refresh method.

While you proposed a new refresh method on the RouterService, which is good, it does not cover this use-case! There must be a way to refresh the current Route from the template/controller.

Why can't you inject the router service into the controller or whatever component backs the template?

luxzeitlos · 2020-05-29T18:32:12Z

The problem is that I only want to refresh the current route, not all parent routes.

sandstrom · 2020-06-30T14:53:36Z

text/0632-deprecate-string-based-actions.md

+
+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.


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.

wagenet · 2022-07-25T14:12:13Z

Is there still interest in moving this forwards?

Gaurav0 · 2022-07-26T05:36:54Z

@ef4 Someone would need to take over this from me.

wagenet · 2022-07-29T23:11:46Z

@chriskrycho do you want to include this in #832?

chriskrycho · 2022-07-30T12:49:08Z

That would make sense to me. I keep forgetting these still exist—it got at least partially cleaned up by removing sendAction for v4.0, just needs the rest done.

wagenet · 2023-01-13T21:23:10Z

Thanks for your work here! This will be addressed as part of #832.

Deprecate string based actions

8fbb172

rwjblue requested changes May 23, 2020

View reviewed changes

Gaurav0 added 3 commits May 23, 2020 14:45

minor tweaks

cd83cab

mention action modifier

3048b99

more detail about send api

67728ad

mehulkar reviewed May 24, 2020

View reviewed changes

change transition path

b6c3382

knownasilya suggested changes May 27, 2020

View reviewed changes

Gaurav0 force-pushed the deprecate_string_based_actions branch from ab4f47e to cdee1bd Compare May 27, 2020 16:50

Apply suggestions from code review

13099a4

Co-authored-by: Ilya Radchenko <knownasilya@gmail.com>

Gaurav0 force-pushed the deprecate_string_based_actions branch from cdee1bd to 13099a4 Compare May 27, 2020 16:56

mehulkar reviewed May 28, 2020

View reviewed changes

text/0632-deprecate-string-based-actions.md Outdated Show resolved Hide resolved

amyrlam mentioned this pull request May 28, 2020

The Ember Times No. 150 - May 29th 2020 ember-learn/ember-blog#660

Merged

14 tasks

Apply suggestions from code review

cde6e6f

Co-authored-by: Mehul Kar <mehul.kar@gmail.com>

patocallaghan reviewed May 28, 2020

View reviewed changes

Gaurav0 added 2 commits May 29, 2020 08:59

add send part to transition path

c9f87cd

transition path for route-action

b50d9c6

amyrlam mentioned this pull request May 30, 2020

The Ember Times No. 151 - June 5th 2020 ember-learn/ember-blog#666

Merged

13 tasks

mehulkar mentioned this pull request Jun 4, 2020

Deprecate string based action handling #629

Closed

amyrlam mentioned this pull request Jun 6, 2020

The Ember Times No. 152 - June 12th 2020 ember-learn/ember-blog#675

Merged

17 tasks

Gaurav0 mentioned this pull request Jun 9, 2020

@route argument #634

Closed

This was referenced Jun 13, 2020

The Ember Times No. 153 - June 19th 2020 ember-learn/ember-blog#682

Merged

The Ember Times No. 154 - June 26th 2020 ember-learn/ember-blog#692

Merged

amyrlam mentioned this pull request Jun 27, 2020

The Ember Times No. 155 - July 10th 2020 ember-learn/ember-blog#702

Merged

21 tasks

sandstrom reviewed Jun 30, 2020

View reviewed changes

amyrlam mentioned this pull request Jul 11, 2020

The Ember Times No. 156 - July 17th 2020 ember-learn/ember-blog#708

Merged

20 tasks

amyrlam mentioned this pull request Jul 18, 2020

The Ember Times No. 157 - July 24th 2020 ember-learn/ember-blog#715

Merged

28 tasks

wagenet added the S-Proposed In the Proposed Stage label Dec 2, 2022

wagenet mentioned this pull request Jan 13, 2023

Deprecate all of Ember Classic #832

Open

wagenet closed this Jan 13, 2023


		## Unresolved questions

		> Can we also deprecate the use of the `actions` hash? Can we also deprecate using `this.actions`?

	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:

Deprecate string based actions #632

Deprecate string based actions #632

Conversation

Gaurav0 commented May 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gaurav0 May 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gaurav0 May 25, 2020 • edited

Choose a reason for hiding this comment

knownasilya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knownasilya May 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehulkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luxzeitlos commented May 29, 2020

Gaurav0 commented May 29, 2020 • edited

luxzeitlos commented May 29, 2020

Choose a reason for hiding this comment

wagenet commented Jul 25, 2022

Gaurav0 commented Jul 26, 2022

wagenet commented Jul 29, 2022

chriskrycho commented Jul 30, 2022

wagenet commented Jan 13, 2023

Gaurav0 commented May 23, 2020 •

edited

Gaurav0 May 24, 2020 •

edited

Gaurav0 May 25, 2020 •

edited

knownasilya May 28, 2020 •

edited

Gaurav0 commented May 29, 2020 •

edited