[BUGFIX beta] [DEPRECATION] Deprecate @foo={{helper}}

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.
emberjs · Apr 21, 2021 · d551570 · d551570
1 parent 21bd70c
commit d551570
Show file tree

Hide file tree

Showing 15 changed files with 321 additions and 166 deletions.
diff --git a/package.json b/package.json
@@ -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.1",
     "babel-plugin-debug-macros": "^0.3.3",
     "babel-plugin-filter-imports": "^4.0.0",
     "broccoli-concat": "^4.2.4",
@@ -75,19 +75,19 @@
   },
   "devDependencies": {
     "@babel/preset-env": "^7.9.5",
-    "@glimmer/compiler": "0.77.5",
+    "@glimmer/compiler": "0.78.1",
     "@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.1",
+    "@glimmer/interfaces": "0.78.1",
+    "@glimmer/manager": "0.78.1",
+    "@glimmer/destroyable": "0.78.1",
+    "@glimmer/owner": "0.78.1",
+    "@glimmer/node": "0.78.1",
+    "@glimmer/opcode-compiler": "0.78.1",
+    "@glimmer/program": "0.78.1",
+    "@glimmer/reference": "0.78.1",
+    "@glimmer/runtime": "0.78.1",
+    "@glimmer/validator": "0.78.1",
     "@simple-dom/document": "^1.4.0",
     "@types/qunit": "^2.9.1",
     "@types/rsvp": "^4.0.3",

diff --git a/packages/@ember/-internals/glimmer/lib/environment.ts b/packages/@ember/-internals/glimmer/lib/environment.ts
@@ -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 }[] = [];

diff --git a/packages/@ember/-internals/glimmer/tests/integration/application/engine-test.js b/packages/@ember/-internals/glimmer/tests/integration/application/engine-test.js
@@ -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);
@@ -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`

diff --git a/packages/@ember/-internals/glimmer/tests/integration/application/rendering-test.js b/packages/@ember/-internals/glimmer/tests/integration/application/rendering-test.js
@@ -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(
@@ -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 () {
@@ -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 {
@@ -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 () {
@@ -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 () {

diff --git a/packages/@ember/-internals/glimmer/tests/integration/components/curly-components-test.js b/packages/@ember/-internals/glimmer/tests/integration/components/curly-components-test.js
@@ -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', {

diff --git a/...@ember/-internals/glimmer/tests/integration/components/link-to/query-params-angle-test.js b/...@ember/-internals/glimmer/tests/integration/components/link-to/query-params-angle-test.js
@@ -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('/');
@@ -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('/');
 

diff --git a/...ages/@ember/-internals/glimmer/tests/integration/components/link-to/routing-angle-test.js b/...ages/@ember/-internals/glimmer/tests/integration/components/link-to/routing-angle-test.js
@@ -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('/');

diff --git a/...s/@ember/-internals/glimmer/tests/integration/components/template-only-components-test.js b/...s/@ember/-internals/glimmer/tests/integration/components/template-only-components-test.js
@@ -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}}|');

diff --git a/packages/@ember/-internals/glimmer/tests/integration/custom-modifier-manager-test.js b/packages/@ember/-internals/glimmer/tests/integration/custom-modifier-manager-test.js
@@ -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';
@@ -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();
+    }
   }
 );
 

diff --git a/packages/@ember/-internals/glimmer/tests/integration/helpers/custom-helper-test.js b/packages/@ember/-internals/glimmer/tests/integration/helpers/custom-helper-test.js
@@ -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');
@@ -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'));
@@ -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();
+      }
+    }
+  );
 }
diff --git a/packages/@ember/-internals/glimmer/tests/integration/helpers/yield-test.js b/packages/@ember/-internals/glimmer/tests/integration/helpers/yield-test.js
@@ -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>',

diff --git a/packages/@ember/-internals/glimmer/tests/integration/syntax/each-test.js b/packages/@ember/-internals/glimmer/tests/integration/syntax/each-test.js
@@ -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']);
@@ -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']);

diff --git a/packages/@ember/-internals/glimmer/tests/integration/syntax/let-test.js b/packages/@ember/-internals/glimmer/tests/integration/syntax/let-test.js
@@ -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}}`, {
@@ -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(
@@ -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(