Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Deprecate string based actions #632
Changes from 6 commits
8fbb172
cd83cab
3048b99
67728ad
b6c3382
13099a4
cde6e6f
c9f87cd
b50d9c6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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}}
.