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

Remove deprecated sendAction #266

Merged

Conversation

Kilowhisky
Copy link
Contributor

Implements #264.

There are two caveats about this code.

  1. It uses placeholder methods instead of directly referencing the functions because bind(this) would break if the user changed the action attached dynamically. Code could be reduced if this is not a worry.
  2. It is not backwards compatible below 1.13.

@RobbieTheWagner
Copy link
Owner

It uses placeholder methods instead of directly referencing the functions because bind(this) would break if the user changed the action attached dynamically. Code could be reduced if this is not a worry.

@Kilowhisky can you say more about this?

I think we could probably just delete these placeholders, but maybe add checks to make sure they are functions before trying to call them.

@Kilowhisky
Copy link
Contributor Author

This style was taken from the deprecation RFC for sendAction.

https://github.com/emberjs/rfcs/blob/master/text/0335-deprecate-send-action.md

The stated use is the following:

// if the salute action is optional you'll have to guard in case it's undefined:
      // if (this.salute) {
      //   this.salute()
      // }
      // 
      // Alternatively, you can also define a noop salute function:
      // salute() {}
      //
      // This allows you to remove the guard while provinding an obvious place to add
      // docs for that action.

Its entirely by style. I also did some testing where did a bind(this) directly to the action method and it worked just fine. But that was under testing and i'm not entirely sure about what would happen if someone tried to change action binding after the component was created. I don't think i've ever done this in code but you never know.

Let me know if i should change it.

@RobbieTheWagner RobbieTheWagner merged commit 4ffecce into RobbieTheWagner:master Sep 23, 2018
@RobbieTheWagner
Copy link
Owner

Makes sense. Thanks for the PR!

@seansellek
Copy link

Can we cut a release now that these deprecations have been removed?

@RobbieTheWagner
Copy link
Owner

@seansellek yeah, I'll try to cut a release soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants