Deprecations/send action #801
Conversation
|
@billybonks can you additionally add a test which passes a closure action? The test would need to only run in versions of Ember which support that, of course. |
There was a problem hiding this comment.
With the suggestions from @mixonic, this looks good to me. Thanks for the PR, @billybonks !
billybonks
force-pushed
the
deprecations/send-action
branch
from
45082dc to
ee6a451
Compare
billybonks
force-pushed
the
deprecations/send-action
branch
2 times, most recently
from
790f8a5 to
ea76150
Compare
| const fullTable = hbs` | ||
| function actionString(actionName) { | ||
| let quotedActionName = `"${actionName}"`; | ||
| return SUPPORTS_CLOSURE_ACTIONS ? `(action ${quotedActionName})` : quotedActionName; |
There was a problem hiding this comment.
Oh dang, this is clever but it is too much.
We need to test both versions of action explicitly, the string version and the closure action. We don't want to test only one of those options based on env.
I'd expect something like:
if (!emberVersionGte('3.0')) { // is 3.0 right for this removal?
test('test sendAction', /* */);
}
if (emberVersionGte('1.13')) {
test('test (action', /* */);
}...that way the 2.x range of Ember versions is testing both versions, and we don't need the runtime compiler and interpolation.
There was a problem hiding this comment.
so the only way to do to your recommendation without runtime compiler. would be to duplicate the fullTable string, since onClick="onRowClick" needs to become onClick=(action "onRowClick") or whichever variant. duplicating has a maintenance overhead that I wanted to avoid.
any idea how we can overcome that challenge?
There was a problem hiding this comment.
@billybonks I think that is the point- we need to have test coverage of both invocation styles. For example you could imagine you even want to test onClick=this.actionViaActionDecorator works. That could only be tested in 3.14(ish, whenever the decorator was added) though.
We could write our own babel plugin which converts based on the current version of Ember at build time:
hbsWithClassicActionTranspilation`{{foo onClick=(action 'onRowClick')}}`
// can, based on a macro/transpilation step, be emitted as either:
hbs`{{foo onClick=(action 'onRowClick')}}`
hbs`{{foo onClick='onRowClick'}}`
// in fact in 3.14+ when the @action decorator is used you could also emit it as
hbs`{{foo onClick=this.onRowClick}}`
// thought I guess that requires a different declaration of the action in JS
// space, so maybe is out of scope for a compilation step.
When tests are run with the 1.12 try scenario it will use non closure actions otherwise it will use closure actions. I had to compile the tempalte string at test run time and not at build time because i wanted to keep the same string instead of duplicating the string. As long as 1.12 is in the try this will test both cases, but if 1.12 is removed then non closure actions wont be tested.
billybonks
force-pushed
the
deprecations/send-action
branch
from
ea76150 to
73fa857
Compare
|
Closed #744 in favor of this. |
|
I'll try finish the request to change the tests today, it's half done at the moment |
|
Seems like this PR got stalled, what’s remaining? Can we push it over the finish line? |
|
I flagged this as |
|
Closing in favour of #860 |
Saw deprecations, so thought i would start to fix them. I created a util that should make it easy to replace in most places. After creating the pr i saw #744 with related work.
would like to see if you are happy with the implementation before i fix the remaining sendAction calls in the code.