Skip to content

Commit

Permalink
[BUGFIX beta] [DEPRECATION] Deprecate @foo={{helper}}
Browse files Browse the repository at this point in the history
Per [RFC 496](https://github.com/emberjs/rfcs/blob/master/text/0496-handlebars-strict-mode.md#3-no-implicit-invocation-of-argument-less-helpers),
this commit deprecates the implicit invocation of argument-less
helpers and requires explicit parenthesis if:

1. Non-strict mode, AND
2. `helper` is a free variable (not `this.helper`, `@helper` or
   a local variable), AND
3. No arguments are provided to the helper, AND
4. It's in a component invocation's named argument position (not
   `<div id={{helper}}>` or `<Foo bar={{helper}}>`), AND
5. Not parenthesized (not `@foo={{(helper)}}`), AND
6. Not interpolated (not `@foo="{{helper}}"`).

The cases in real apps where all of the above are true should be
quite rare, as most helpers take at least one argument.

This is probably not the most efficient/minimal changes to the
compiler infrastructure, but I chose to optimize for making the
change easier to remove/rollback (i.e. adding new nodes instead
of changing existing ones) as we won't be needing this for very
long.

This also bumps `@glimmer/*` to latest which also brings in the
bugfix commit glimmerjs/glimmer-vm#1293.
  • Loading branch information
chancancode committed Apr 21, 2021
1 parent 21bd70c commit 0e148ac
Show file tree
Hide file tree
Showing 14 changed files with 299 additions and 166 deletions.
26 changes: 13 additions & 13 deletions package.json
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.0",
"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.0",
"@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.0",
"@glimmer/interfaces": "0.78.0",
"@glimmer/manager": "0.78.0",
"@glimmer/destroyable": "0.78.0",
"@glimmer/owner": "0.78.0",
"@glimmer/node": "0.78.0",
"@glimmer/opcode-compiler": "0.78.0",
"@glimmer/program": "0.78.0",
"@glimmer/reference": "0.78.0",
"@glimmer/runtime": "0.78.0",
"@glimmer/validator": "0.78.0",
"@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
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
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
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
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
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
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
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
@@ -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
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.

['@skip 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();
}
}
);
}
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
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
Expand Up @@ -106,7 +106,7 @@ moduleFor(

['@test the scoped variable is not available outside the {{#with}} 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}}-{{#with this.other as |name|}}{{name}}{{/with}}-{{name}}`, {
Expand Down Expand Up @@ -295,7 +295,7 @@ moduleFor(

['@test the scoped variable is not available outside the {{#with}} 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 @@ -380,7 +380,7 @@ moduleFor(

['@test nested {{#with}} 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 0e148ac

Please sign in to comment.