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

Jump to bottom

[DEPRECATION]Deprecate send action (RFC #335) #16744

Merged
merged 1 commit into from Jun 22, 2018
Merged

[DEPRECATION]Deprecate send action (RFC #335) #16744

merged 1 commit into from Jun 22, 2018

Conversation

cibernox
Copy link
Contributor

@cibernox cibernox commented Jun 12, 2018 •
edited

Ready for review.

rwjblue
rwjblue reviewed Jun 12, 2018
View reviewed changes
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! I left an inline comment RE: including more identifiable info in the deprecation message, and I'd like to see the sendAction support code be wrapped for svelting as part of this PR.

The svelte wrapping stuff is pretty new, but should hopefully be easy to add. I'll dig up an example...

packages/ember-views/lib/mixins/action_support.js Outdated
@@ -116,6 +116,10 @@ export default Mixin.create({
`Attempted to call .sendAction() with the action '${action}' on the destroyed object '${this}'.`,
!this.isDestroying && !this.isDestroyed
);
deprecate('Component#sendAction is deprecated. Please use closure actions instead.', false, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should include this somewhere in the deprecation message (to help track down where the deprecation is coming from).

@rwjblue
Copy link
Member

rwjblue commented Jun 12, 2018

See #16745 for an example of the work needed to svelte...

cibernox
cibernox commented Jun 13, 2018
View reviewed changes
packages/ember-views/lib/mixins/text_support.js Outdated
deprecate(
`Passing actions to components as strings (like {{input ${eventName}="${actionName}"}}) is deprecated. Please use closure actions instead ({{input ${eventName}=(action "${actionName}")}})`,
false,
{ id: 'ember-input.send-action', until: '4.0.0' }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: Use the same id than in the sendAction deprecation even if the text is different, as in it's essence it's the same feature.

@cibernox cibernox changed the title [DEPRECATION][WIP]Deprecate send action (RFC #335) [DEPRECATION]Deprecate send action (RFC #335) Jun 15, 2018
@cibernox
Copy link
Contributor Author

@rwjblue I've implemented the AST deprecation and tested it. I've also wrapped the sendAction helper for svelte removal.

Points of attention:

  1. Should the AST deprecation to have the same ID as the runtime deprecation?
  2. In the file of constants for svelte, what's the version number I should use?

@cibernox
Copy link
Contributor Author

PR for the deprecation entry: ember-learn/deprecation-app#146

@cibernox cibernox force-pushed the deprecate-send-action branch 3 times, most recently from 382ef10 to 1f28a7b Compare June 15, 2018 20:43
rwjblue
rwjblue reviewed Jun 18, 2018
View reviewed changes
packages/ember-glimmer/tests/integration/helpers/input-test.js Outdated
@@ -271,10 +271,30 @@ moduleFor(
// this.assertSelectionRange(8, 8); //NOTE: this fails in IE, the range is 0 -> 0 (TEST_SUITE=sauce)
}

['@test sends an action with `{{input enter="foo"}}` when <enter> is pressed'](assert) {
assert.expect(1);
['@test sends an action with `{{input enter="foo"}}` when <enter> is pressed with deprecation'](
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we tweak the tests that we now consider deprecated to have [DEPRECATED] in the test title (instead of in this case with deprecation)?

It will make future removal much easier...

packages/ember-template-compiler/lib/plugins/deprecate-render.ts Outdated
@@ -3,7 +3,6 @@ import { RENDER_HELPER } from '@ember/deprecated-features';
import { AST, ASTPlugin, ASTPluginEnvironment } from '@glimmer/syntax';
import calculateLocationDisplay from '../system/calculate-location-display';

// Remove after 3.4 once _ENABLE_RENDER_SUPPORT flag is no longer needed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think this comment is still valid...

packages/ember-template-compiler/lib/plugins/deprecate-send-action.ts Outdated
'key-down',
];

export default function deprecateRender(env: ASTPluginEnvironment): ASTPlugin | undefined {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you tweak the function name here (it current is deprecateRender)?

packages/ember-template-compiler/lib/plugins/index.ts Outdated
@@ -40,6 +41,7 @@ const transforms: Array<APluginFunc> = [
TransformInElement,
AssertIfHelperWithoutArguments,
AssertSplattributeExpressions,
DeprecateSendAction,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the RENDER_HELPER usage in this file, can you move this down into a conditional? That will allow us to strip all of the plugin code when an app says they are compliant with 3.5 (aka svelte)...

packages/ember-template-compiler/lib/plugins/deprecate-send-action.ts Outdated
];

export default function deprecateRender(env: ASTPluginEnvironment): ASTPlugin | undefined {
let { moduleName } = env.meta;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you wrap the "guts" of this function in if (SEND_ACTION) { for svelting purposes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If in packages/ember-template-compiler/lib/plugins/index.ts I only conditionally push the transform, what's the point of wrapping the "guts" yet again on the same condition?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file size reduction

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this AST transforms were not shipped as templates are compiled to JSON.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are correct, they are not shipped for a default app, but it is still "a thing" to ship the template compiler at runtime so that you can compile dynamic templates

packages/ember-views/lib/mixins/text_support.js Outdated
let value = get(view, 'value');

view.sendAction(eventName, value);
if (typeof actionName === 'string') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add SEND_ACTION to this conditional?

So it would become:

if (SEND_ACTION && typeof actionName === 'string') {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, how does this guard support?

{{input action='some-generic-action'}}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does {{input action='some-generic-action'}} do?

@cibernox cibernox force-pushed the deprecate-send-action branch 4 times, most recently from b21f77b to ab4b623 Compare June 19, 2018 12:24
[DEPRECATION] Deprecate Component#sendAction (RFC emberjs#335)
10f876d 
This also implicitly deprecated passing actions to the built-in inputs like `{{input enter="foo"}},
instead users must do `{{input enter=(action "foo")}}`.
There is a build-time deprecation that uses AST visitors to output precise information of file
and line the offending code is.
There is also a runtime-deprecation for cases that the AST deprecation can't catch.
@cibernox
Copy link
Contributor Author

@rwjblue can you rebuild the browserstack job? I think it's a false negative.

@rwjblue
Copy link
Member

rwjblue commented Jun 19, 2018

Done!

@cibernox
Copy link
Contributor Author

Green!

@rwjblue
Copy link
Member

rwjblue commented Jun 22, 2018

The deprecation RFC has been merged now, so this is good to go.

Thanks @cibernox!

@rwjblue rwjblue merged commit 6f64dec into emberjs:master Jun 22, 2018
@cibernox cibernox deleted the deprecate-send-action branch June 22, 2018 18:58
@cibernox
Copy link
Contributor Author

@rwjblue the docs also need a push 😄
ember-learn/deprecation-app#146

@rwjblue rwjblue mentioned this pull request Oct 13, 2018
Merged
@rwjblue rwjblue mentioned this pull request Jan 23, 2019
Closed
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwjblue rwjblue rwjblue approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@cibernox @rwjblue