Skip to content

Commit

Permalink
[BUGFIX beta] Local variables should win over helpers
Browse files Browse the repository at this point in the history
```hbs
{{#let this.foo as |foo|}}
  {{foo 1 2 3}}
    ~~~ local variable invocation!
{{/let}}
```

Previously, this would have tried to resolve and invoke the helper `foo`,
ignoring the presence of the `foo` local variable. This is inconsistent
with our general lexical lookup rules. This will now invoke `foo` local
variable as a contextual component.

In other words, this removes the remaining "dot rule" exceptions for
contextual components, as per [RFC #311](https://github.com/emberjs/rfcs/blob/master/text/0311-angle-bracket-invocation.md):

> We propose to relax that rule to match the proposed angle bracket
> invocation semantics (i.e. allowing local variables without a dot, as
> well as `@names`, but disallowing implicit `this` lookup).

This commit also fixed another related issue:

```hbs
{{#let (hash foo="foo-bar") as |h|}}
  {{h.foo 1 2 3}}
    ~~~~~ this is a string, not a component!
{{/let}}
```

Previously, this would have tried to resolve and invoke the `foo-bar`
component. This is an unintended consequence (i.e. a bug) of the "dot
rule" implementation. This will now raise a `TypeError` in development
mode and result in undefined behavior in production builds (probably
some other runtime error deep inside the Glimmer VM internals).

Fixes #17121, #16510.
  • Loading branch information
chancancode committed Oct 19, 2018
1 parent fa91986 commit 1aa4fbb
Show file tree
Hide file tree
Showing 7 changed files with 387 additions and 108 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { DEBUG } from '@glimmer/env';
import { Opaque } from '@glimmer/interfaces';
import { Tag, VersionedPathReference } from '@glimmer/reference';
import { Arguments, Helper, VM } from '@glimmer/runtime';
import { Maybe } from '@glimmer/util';

let helper: Maybe<Helper> = undefined;

if (DEBUG) {
class ComponentAssertionReference implements VersionedPathReference<Opaque> {
public tag: Tag;

constructor(private component: VersionedPathReference<Opaque>, private message: string) {
this.tag = component.tag;
}

value(): Opaque {
let value = this.component.value();

if (typeof value === 'string') {
throw new TypeError(this.message);
}

return value;
}

get(property: string): VersionedPathReference<Opaque> {
return this.component.get(property);
}
}

helper = (_vm: VM, args: Arguments) =>
new ComponentAssertionReference(args.positional.at(0), args.positional.at(1).value() as string);
}

export default helper;
6 changes: 6 additions & 0 deletions packages/@ember/-internals/glimmer/lib/resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { lookupComponent, lookupPartial, OwnedTemplateMeta } from '@ember/-inter
import { EMBER_MODULE_UNIFICATION, GLIMMER_CUSTOM_COMPONENT_MANAGER } from '@ember/canary-features';
import { assert } from '@ember/debug';
import { _instrumentStart } from '@ember/instrumentation';
import { DEBUG } from '@glimmer/env';
import {
ComponentDefinition,
Opaque,
Expand All @@ -18,6 +19,7 @@ import { CurlyComponentDefinition } from './component-managers/curly';
import { CustomManagerDefinition, ManagerDelegate } from './component-managers/custom';
import { TemplateOnlyComponentDefinition } from './component-managers/template-only';
import { isHelperFactory, isSimpleHelper } from './helper';
import { default as componentAssertionHelper } from './helpers/-assert-implicit-component-helper-argument';
import { default as classHelper } from './helpers/-class';
import { default as htmlSafeHelper } from './helpers/-html-safe';
import { default as inputTypeHelper } from './helpers/-input-type';
Expand Down Expand Up @@ -76,6 +78,10 @@ const BUILTINS_HELPERS = {
'-outlet': outletHelper,
};

if (DEBUG) {
BUILTINS_HELPERS['-assert-implicit-component-helper-argument'] = componentAssertionHelper;
}

const BUILTIN_MODIFIERS = {
action: { manager: new ActionModifierManager(), state: null },
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1387,6 +1387,98 @@ moduleFor(
this.render('{{component (component "textarea" type="text")}}');
}, 'You cannot use `textarea` as a component name.');
}

['@test GH#17121 local variable should win over helper (without arguments)']() {
this.registerHelper('foo', () => 'foo helper');

this.registerComponent('foo-bar', { template: 'foo-bar component' });

this.render(strip`
{{#let (component 'foo-bar') as |foo|}}
{{foo}}
{{/let}}
`);

this.assertText('foo-bar component');

this.assertStableRerender();
}

['@test GH#17121 local variable should win over helper (with arguments)']() {
this.registerHelper('foo', params => `foo helper: ${params.join(' ')}`);

this.registerComponent('foo-bar', {
ComponentClass: Component.extend().reopenClass({
positionalParams: 'params',
}),
template: 'foo-bar component:{{#each params as |param|}} {{param}}{{/each}}',
});

this.render(strip`
{{#let (component 'foo-bar') as |foo|}}
{{foo 1 2 3}}
{{/let}}
`);

this.assertText('foo-bar component: 1 2 3');

this.assertStableRerender();
}

['@test GH#17121 implicit component invocations should not perform string lookup'](assert) {
this.registerComponent('foo-bar', { template: 'foo-bar component' });

assert.throws(
() =>
this.render(strip`
{{#let 'foo-bar' as |foo|}}
{{foo 1 2 3}}
{{/let}}
`),
new TypeError(
"expected `foo` to be a contextual component but found a string. Did you mean `(component foo)`? ('-top-level' @ L1:C29) "
)
);
}

['@test RFC#311 invoking named args (without arguments)']() {
this.registerComponent('x-outer', { template: '{{@inner}}' });
this.registerComponent('x-inner', { template: 'inner' });

this.render('{{x-outer inner=(component "x-inner")}}');

this.assertText('inner');

this.assertStableRerender();
}

['@test RFC#311 invoking named args (with arguments)']() {
this.registerComponent('x-outer', { template: '{{@inner 1 2 3}}' });

this.registerComponent('x-inner', {
ComponentClass: Component.extend().reopenClass({
positionalParams: 'params',
}),
template: 'inner:{{#each params as |param|}} {{param}}{{/each}}',
});

this.render('{{x-outer inner=(component "x-inner")}}');

this.assertText('inner: 1 2 3');

this.assertStableRerender();
}

['@test RFC#311 invoking named args (with a block)']() {
this.registerComponent('x-outer', { template: '{{#@inner}}outer{{/@inner}}' });
this.registerComponent('x-inner', { template: 'inner {{yield}}' });

this.render('{{x-outer inner=(component "x-inner")}}');

this.assertText('inner outer');

this.assertStableRerender();
}
}
);

Expand Down
4 changes: 2 additions & 2 deletions packages/ember-template-compiler/lib/plugins/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import DeprecateSendAction from './deprecate-send-action';
import TransformActionSyntax from './transform-action-syntax';
import TransformAngleBracketComponents from './transform-angle-bracket-components';
import TransformAttrsIntoArgs from './transform-attrs-into-args';
import TransformDotComponentInvocation from './transform-dot-component-invocation';
import TransformComponentInvocation from './transform-component-invocation';
import TransformEachInIntoEach from './transform-each-in-into-each';
import TransformHasBlockSyntax from './transform-has-block-syntax';
import TransformInElement from './transform-in-element';
Expand All @@ -23,7 +23,7 @@ import { ASTPlugin, ASTPluginEnvironment } from '@glimmer/syntax';
export type APluginFunc = (env: ASTPluginEnvironment) => ASTPlugin | undefined;

const transforms: Array<APluginFunc> = [
TransformDotComponentInvocation,
TransformComponentInvocation,
TransformAngleBracketComponents,
TransformTopLevelComponents,
TransformInlineLinkTo,
Expand Down
Loading

0 comments on commit 1aa4fbb

Please sign in to comment.