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 3 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.
This is missing quite a bit of info I think.
.send
?this.actions
(this is a thing some folks have done, and the@action
decorator still populates this actions hash)?actions
hash?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.
Added to unresolved questions for the moment.
Added to unresolved questions for the moment.
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.
Added.
Partially added.
Help wanted here.
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.
This should use
@action
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.
@action
will not work with.extend(
; i.e. it would require a native class.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.
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):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.
I'm doubtful we should be recommending this. This syntax is not shown in any guides, pre- or post-Octane, and will be unfamiliar.
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.
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!
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 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.
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.
Ok. I'm putting this in. Where in the guides can we discuss this?
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.
Not 100% sure, maybe a "classic interop" thing in the advanced topics?
@emberjs/learning-team-managers - Thoughts?
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.
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?
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.
Do you have an example RFC that does this?
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.
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 theactions
key on a class no longer holds any special significance.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.
I think I agree. Would you be willing to PR this PR and help out? Probably belongs in "How we teach this" section.