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 20, 2021
1 parent 21bd70c commit 33b9227
Show file tree
Hide file tree
Showing 5 changed files with 278 additions and 145 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
@@ -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 @@ -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();
}
}
);
}

0 comments on commit 33b9227

Please sign in to comment.