Skip to content

Commit

Permalink
Merge pull request #19499 from emberjs/deprecate-argument-less-helper…
Browse files Browse the repository at this point in the history 
…-paren-less-invocation

[BUGFIX beta] [DEPRECATION] Deprecate `@foo={{helper}}`
  • Loading branch information
@wycats
wycats committed Apr 22, 2021
2 parents 21bd70c + 62e2816 commit a6a6833
Show file tree
Hide file tree
Showing 15 changed files with 321 additions and 166 deletions.
26 changes: 13 additions & 13 deletions package.json
View file
Expand Up @@ -51,7 +51,7 @@
"@babel/plugin-transform-block-scoping": "^7.8.3",
"@babel/plugin-transform-object-assign": "^7.8.3",
"@ember/edition-utils": "^1.2.0",
"@glimmer/vm-babel-plugins": "0.77.5",
"@glimmer/vm-babel-plugins": "0.78.2",
"babel-plugin-debug-macros": "^0.3.3",
"babel-plugin-filter-imports": "^4.0.0",
"broccoli-concat": "^4.2.4",
Expand All @@ -75,19 +75,19 @@
},
"devDependencies": {
"@babel/preset-env": "^7.9.5",
"@glimmer/compiler": "0.77.5",
"@glimmer/compiler": "0.78.2",
"@glimmer/env": "^0.1.7",
"@glimmer/global-context": "0.77.5",
"@glimmer/interfaces": "0.77.5",
"@glimmer/manager": "0.77.5",
"@glimmer/destroyable": "0.77.5",
"@glimmer/owner": "0.77.5",
"@glimmer/node": "0.77.5",
"@glimmer/opcode-compiler": "0.77.5",
"@glimmer/program": "0.77.5",
"@glimmer/reference": "0.77.5",
"@glimmer/runtime": "0.77.5",
"@glimmer/validator": "0.77.5",
"@glimmer/global-context": "0.78.2",
"@glimmer/interfaces": "0.78.2",
"@glimmer/manager": "0.78.2",
"@glimmer/destroyable": "0.78.2",
"@glimmer/owner": "0.78.2",
"@glimmer/node": "0.78.2",
"@glimmer/opcode-compiler": "0.78.2",
"@glimmer/program": "0.78.2",
"@glimmer/reference": "0.78.2",
"@glimmer/runtime": "0.78.2",
"@glimmer/validator": "0.78.2",
"@simple-dom/document": "^1.4.0",
"@types/qunit": "^2.9.1",
"@types/rsvp": "^4.0.3",
Expand Down
8 changes: 8 additions & 0 deletions packages/@ember/-internals/glimmer/lib/environment.ts
View file
Expand Up @@ -116,6 +116,14 @@ const VM_DEPRECATION_OVERRIDES: (DeprecationOptions & {
enabled: '3.26.0',
},
},
{
id: 'argument-less-helper-paren-less-invocation',
until: '4.0.0',
for: 'ember-source',
since: {
enabled: '3.27.0',
},
},
];

const VM_ASSERTION_OVERRIDES: { id: string; message: string }[] = [];
Expand Down
4 changes: 2 additions & 2 deletions packages/@ember/-internals/glimmer/tests/integration/application/engine-test.js
View file
Expand Up @@ -294,7 +294,7 @@ moduleFor(
`);

expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
/The `[^`]+` property(?: path)? was used in the `[^`]+` template without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.add('template:application', sharedTemplate);
Expand Down Expand Up @@ -345,7 +345,7 @@ moduleFor(
this.assert.expect(2);

expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
/The `[^`]+` property(?: path)? was used in the `[^`]+` template without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

let sharedLayout = compile(strip`
Expand Down
10 changes: 5 additions & 5 deletions packages/@ember/-internals/glimmer/tests/integration/application/rendering-test.js
View file
Expand Up @@ -107,7 +107,7 @@ moduleFor(

['@test it can access the model provided by the route via implicit this fallback']() {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
/The `[^`]+` property(?: path)? was used in the `[^`]+` template without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.add(
Expand Down Expand Up @@ -143,7 +143,7 @@ moduleFor(

async ['@test interior mutations on the model with set'](assert) {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
/The `[^`]+` property(?: path)? was used in the `[^`]+` template without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.router.map(function () {
Expand Down Expand Up @@ -204,7 +204,7 @@ moduleFor(

async ['@test interior mutations on the model with tracked properties'](assert) {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
/The `[^`]+` property(?: path)? was used in the `[^`]+` template without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

class Model {
Expand Down Expand Up @@ -272,7 +272,7 @@ moduleFor(

async ['@test exterior mutations on the model with set'](assert) {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
/The `[^`]+` property(?: path)? was used in the `[^`]+` template without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.router.map(function () {
Expand Down Expand Up @@ -333,7 +333,7 @@ moduleFor(

async ['@test exterior mutations on the model with tracked properties'](assert) {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
/The `[^`]+` property(?: path)? was used in the `[^`]+` template without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.router.map(function () {
Expand Down
2 changes: 1 addition & 1 deletion packages/@ember/-internals/glimmer/tests/integration/components/curly-components-test.js
View file
Expand Up @@ -3883,7 +3883,7 @@ moduleFor(

['@test can use `{{component.foo}}` in a template GH#19313']() {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
/The `[^`]+` property(?: path)? was used in the `[^`]+` template without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.registerComponent('foo-bar', {
Expand Down
4 changes: 2 additions & 2 deletions ...@ember/-internals/glimmer/tests/integration/components/link-to/query-params-angle-test.js
View file
Expand Up @@ -144,7 +144,7 @@ moduleFor(
) {
this.addTemplate(
'index',
`<LinkTo id='the-link' @route='index' @query={{hash}}>Index</LinkTo>`
`<LinkTo id='the-link' @route='index' @query={{(hash)}}>Index</LinkTo>`
);

await this.visit('/');
Expand All @@ -163,7 +163,7 @@ moduleFor(
async [`@test it doesn't update controller QP properties on current route when invoked (empty query-params obj, inferred route)`](
assert
) {
this.addTemplate('index', `<LinkTo id='the-link' @query={{hash}}>Index</LinkTo>`);
this.addTemplate('index', `<LinkTo id='the-link' @query={{(hash)}}>Index</LinkTo>`);

await this.visit('/');

Expand Down
2 changes: 1 addition & 1 deletion ...ages/@ember/-internals/glimmer/tests/integration/components/link-to/routing-angle-test.js
View file
Expand Up @@ -2085,7 +2085,7 @@ moduleFor(

this.addTemplate(
'index',
`<LinkTo id='the-link' @route='index' @query={{hash}}>Index</LinkTo>`
`<LinkTo id='the-link' @route='index' @query={{(hash)}}>Index</LinkTo>`
);

await this.visit('/');
Expand Down
2 changes: 1 addition & 1 deletion ...s/@ember/-internals/glimmer/tests/integration/components/template-only-components-test.js
View file
Expand Up @@ -198,7 +198,7 @@ if (ENV._TEMPLATE_ONLY_GLIMMER_COMPONENTS) {

['@test it renders named arguments as reflected properties']() {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
/The `[^`]+` property(?: path)? was used in the `[^`]+` template without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.registerTemplateOnlyComponent('foo-bar', '|{{foo}}|{{this.bar}}|');
Expand Down
22 changes: 21 additions & 1 deletion packages/@ember/-internals/glimmer/tests/integration/custom-modifier-manager-test.js
View file
@@ -1,5 +1,11 @@
import { DEBUG } from '@glimmer/env';
import { moduleFor, RenderingTestCase, runTask, defineSimpleModifier } from 'internal-test-helpers';
import {
moduleFor,
RenderingTestCase,
runTask,
defineSimpleHelper,
defineSimpleModifier,
} from 'internal-test-helpers';

import { Component } from '@ember/-internals/glimmer';
import { setModifierManager, modifierCapabilities } from '@glimmer/manager';
Expand Down Expand Up @@ -644,6 +650,20 @@ moduleFor(
});
}, /Cannot use the \(modifier\) keyword yet, as it has not been implemented/);
}

'@feature(EMBER_DYNAMIC_HELPERS_AND_MODIFIERS) Can use a dynamic modifier with a nested dynamic helper'() {
let foo = defineSimpleHelper(() => 'Hello, world!');
let bar = defineSimpleModifier((element, [value]) => (element.innerHTML = value));

this.registerComponent('baz', {
template: '<div {{this.bar (this.foo)}}></div>',
ComponentClass: Component.extend({ tagName: '', foo, bar }),
});

this.render('<Baz/>');
this.assertHTML('<div>Hello, world!</div>');
this.assertStableRerender();
}
}
);

Expand Down
107 changes: 106 additions & 1 deletion packages/@ember/-internals/glimmer/tests/integration/helpers/custom-helper-test.js
View file
Expand Up @@ -36,7 +36,7 @@ moduleFor(

['@test it does not resolve helpers with a `.` (period)']() {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
/The `[^`]+` property(?: path)? was used in the `[^`]+` template without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.registerHelper('hello.world', () => 'hello world');
Expand Down Expand Up @@ -826,6 +826,20 @@ moduleFor(
}, /Cannot use the \(helper\) keyword yet, as it has not been implemented/);
}

'@feature(EMBER_DYNAMIC_HELPERS_AND_MODIFIERS) Can use a dynamic helper with nested helpers'() {
let foo = defineSimpleHelper(() => 'world!');
let bar = defineSimpleHelper((value) => 'Hello, ' + value);

this.registerComponent('baz', {
template: '{{this.bar (this.foo)}}',
ComponentClass: Component.extend({ foo, bar }),
});

this.render('<Baz/>');
this.assertText('Hello, world!');
this.assertStableRerender();
}

['@test helpers are not computed eagerly when used with if expressions'](assert) {
this.registerHelper('is-ok', () => 'hello');
this.registerHelper('throws-error', () => assert.ok(false, 'helper was computed eagerly'));
Expand Down Expand Up @@ -909,4 +923,95 @@ if (DEBUG) {
}
}
);

moduleFor(
'Helpers test: argument-less helper invocation in named arguments position',
class extends RenderingTestCase {
constructor() {
super(...arguments);

this.registerComponent('bar', {
template: '[{{is-string @content}}][{{@content}}]',
});

this.registerHelper('is-string', ([value]) => typeof value === 'string');
}

['@test invoking an argument-less helper without parens in named argument position is deprecated']() {
this.registerHelper('foo', () => 'Hello, world!');

expectDeprecation(
() => this.render('<Bar @content={{foo}} />', { foo: 'Not it!' }),
new RegExp(
/The `foo` helper was used in the `-top-level` template as /.source +
/`@content={{foo}}`\. This is ambigious between wanting the `@content` /.source +
/argument to be the `foo` helper itself, or the result of invoking the /.source +
/`foo` helper \(current behavior\)\. This implicit invocation behavior /.source +
/has been deprecated\./.source
)
);

this.assertText('[true][Hello, world!]');
this.assertStableRerender();
}

['@test invoking an argument-less helper with parens in named argument position is not deprecated']() {
this.registerHelper('foo', () => 'Hello, world!');

expectNoDeprecation(() => this.render('<Bar @content={{(foo)}} />', { foo: 'Not it!' }));

this.assertText('[true][Hello, world!]');
this.assertStableRerender();
}

['@test invoking an argument-less helper with quotes in named argument position is not deprecated']() {
this.registerHelper('foo', () => 'Hello, world!');

expectNoDeprecation(() => this.render('<Bar @content="{{foo}}" />', { foo: 'Not it!' }));

this.assertText('[true][Hello, world!]');
this.assertStableRerender();
}

['@test passing a local helper in named argument position is not deprecated']() {
let foo = defineSimpleHelper(() => 'Hello, world!');

expectNoDeprecation(() =>
this.render(`{{#let this.foo as |foo|}}<Bar @content={{foo}} />{{/let}}`, { foo })
);

this.assertText('[false][Hello, world!]');
this.assertStableRerender();
}

// TODO: this one really should work, and there is a passing test in glimmer-vm,
// but somehow it doesn't work here. This is almost certainly a VM bug as something
// is trying to call `block.compile()` but `block` is the reference for `this.foo`.
// So the execution stack is probably off-by-one or something.

['@test invoking a local helper with parens in named argument position is not deprecated']() {
let foo = defineSimpleHelper(() => 'Hello, world!');

expectNoDeprecation(() =>
this.render(`{{#let this.foo as |foo|}}<Bar @content={{(foo)}} />{{/let}}`, { foo })
);

this.assertText('[true][Hello, world!]');
this.assertStableRerender();
}

// TODO: this one doesn't work yet, and there is a failing test in glimmer-vm

['@skip invoking a helper with quotes in named argument position is not deprecated']() {
let foo = defineSimpleHelper(() => 'Hello, world!');

expectNoDeprecation(() =>
this.render(`{{#let this.foo as |foo|}}<Bar @content="{{foo}}" />{{/let}}`, { foo })
);

this.assertText('[true][Hello, world!]');
this.assertStableRerender();
}
}
);
}
22 changes: 22 additions & 0 deletions packages/@ember/-internals/glimmer/tests/integration/helpers/yield-test.js
View file
Expand Up @@ -53,6 +53,28 @@ moduleFor(
this.assertText('[In layout:] [In block:] Seattle');
}

['@feature(EMBER_NAMED_BLOCKS) <:else> and <:inverse> named blocks']() {
this.registerComponent('yielder', {
template:
'[:else][{{has-block "else"}}][{{yield to="else"}}]' +
'[:inverse][{{has-block "inverse"}}][{{yield to="inverse"}}]',
});

this.render(
'[<Yielder />]' +
'[<Yielder><:else>Hello</:else></Yielder>]' +
'[<Yielder><:inverse>Goodbye</:inverse></Yielder>]'
);

this.assertText(
'[[:else][false][][:inverse][false][]]' +
'[[:else][true][Hello][:inverse][true][Hello]]' +
'[[:else][true][Goodbye][:inverse][true][Goodbye]]'
);

this.assertStableRerender();
}

['@test templates should yield to block inside a nested component']() {
this.registerComponent('outer-comp', {
template: '<div>[In layout:] {{yield}}</div>',
Expand Down
4 changes: 2 additions & 2 deletions packages/@ember/-internals/glimmer/tests/integration/syntax/each-test.js
View file
Expand Up @@ -732,7 +732,7 @@ class EachTest extends AbstractEachTest {

['@test the scoped variable is not available outside the {{#each}} block.']() {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
/The `[^`]+` property(?: path)? was used in the `[^`]+` template without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.makeList(['Yehuda']);
Expand Down Expand Up @@ -953,7 +953,7 @@ class EachTest extends AbstractEachTest {

['@test the scoped variable is not available outside the {{#each}} block']() {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
/The `[^`]+` property(?: path)? was used in the `[^`]+` template without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

let first = this.createList(['Limbo']);
Expand Down
6 changes: 3 additions & 3 deletions packages/@ember/-internals/glimmer/tests/integration/syntax/let-test.js
View file
Expand Up @@ -97,7 +97,7 @@ moduleFor(

['@test the scoped variable is not available outside the {{#let}} block.']() {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
/The `[^`]+` property(?: path)? was used in the `[^`]+` template without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.render(`{{name}}-{{#let this.other as |name|}}{{name}}{{/let}}-{{name}}`, {
Expand Down Expand Up @@ -247,7 +247,7 @@ moduleFor(

['@test the scoped variable is not available outside the {{#let}} block']() {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
/The `[^`]+` property(?: path)? was used in the `[^`]+` template without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.render(
Expand Down Expand Up @@ -332,7 +332,7 @@ moduleFor(

['@test nested {{#let}} blocks should have access to root context']() {
expectDeprecation(
/The `[^`]+` property(?: path)? was used in a template for the `[^`]+` component without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
/The `[^`]+` property(?: path)? was used in the `[^`]+` template without using `this`. This fallback behavior has been deprecated, all properties must be looked up on `this` when used in the template: {{[^}]+}}/
);

this.render(
Expand Down

0 comments on commit a6a6833

Please sign in to comment.