-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Local variables should win over helpers
```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).) 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.
- Loading branch information
1 parent
333a67e
commit 3da51a8
Showing
7 changed files
with
311 additions
and
105 deletions.
There are no files selected for viewing
36 changes: 36 additions & 0 deletions
36
packages/@ember/-internals/glimmer/lib/helpers/-assert-implicit-component-helper-argument.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
212 changes: 212 additions & 0 deletions
212
packages/ember-template-compiler/lib/plugins/transform-component-invocation.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,212 @@ | ||
import { DEBUG } from '@glimmer/env'; | ||
import { AST, ASTPlugin, ASTPluginEnvironment } from '@glimmer/syntax'; | ||
import calculateLocationDisplay from '../system/calculate-location-display'; | ||
import { Builders } from '../types'; | ||
|
||
/** | ||
Transforms unambigious invocations of closure components to be wrapped with | ||
the component helper. | ||
```handlebars | ||
{{!-- this.foo is not a legal helper/component name --}} | ||
{{this.foo "with" some="args"}} | ||
``` | ||
with | ||
```handlebars | ||
{{component this.foo "with" some="args"}} | ||
``` | ||
and | ||
```handlebars | ||
{{!-- this.foo is not a legal helper/component name --}} | ||
{{#this.foo}}...{{/this.foo}} | ||
``` | ||
with | ||
```handlebars | ||
{{#component this.foo}}...{{/component}} | ||
``` | ||
and | ||
```handlebars | ||
{{!-- foo.bar is not a legal helper/component name --}} | ||
{{foo.bar "with" some="args"}} | ||
``` | ||
with | ||
```handlebars | ||
{{component foo.bar "with" some="args"}} | ||
``` | ||
and | ||
```handlebars | ||
{{!-- foo.bar is not a legal helper/component name --}} | ||
{{#foo.bar}}...{{/foo.bar}} | ||
``` | ||
with | ||
```handlebars | ||
{{#component foo.bar}}...{{/component}} | ||
``` | ||
and | ||
```handlebars | ||
{{#let ... as |foo|}} | ||
{{!-- foo is a local variable --}} | ||
{{foo "with" some="args"}} | ||
{{/let}} | ||
``` | ||
with | ||
```handlebars | ||
{{#let ... as |foo|}} | ||
{{component foo "with" some="args"}} | ||
{{/let}} | ||
``` | ||
and | ||
```handlebars | ||
{{#let ... as |foo|}} | ||
{{!-- foo is a local variable --}} | ||
{{#foo}}...{{/foo}} | ||
{{/let}} | ||
``` | ||
with | ||
```handlebars | ||
{{#let ... as |foo|}} | ||
{{#component foo}}...{{/component}} | ||
{{/let}} | ||
``` | ||
@private | ||
@class TransFormComponentInvocation | ||
*/ | ||
export default function transformComponentInvocation(env: ASTPluginEnvironment): ASTPlugin { | ||
let { moduleName } = env.meta; | ||
let { builders: b } = env.syntax; | ||
let locals: string[][] = []; | ||
|
||
return { | ||
name: 'transform-component-invocation', | ||
|
||
visitor: { | ||
ElementNode: { | ||
enter(node: AST.ElementNode) { | ||
locals.push(node.blockParams); | ||
}, | ||
|
||
exit() { | ||
locals.pop(); | ||
}, | ||
}, | ||
|
||
BlockStatement: { | ||
enter(node: AST.BlockStatement) { | ||
// check this before we push the new locals | ||
if (isBlockInvocation(node, locals)) { | ||
wrapInComponent(moduleName, node, b); | ||
} | ||
|
||
locals.push(node.program.blockParams); | ||
}, | ||
|
||
exit() { | ||
locals.pop(); | ||
}, | ||
}, | ||
|
||
MustacheStatement(node: AST.MustacheStatement): AST.Node | void { | ||
if (isInlineInvocation(node, locals)) { | ||
wrapInComponent(moduleName, node, b); | ||
} | ||
}, | ||
}, | ||
}; | ||
} | ||
|
||
function isInlineInvocation(node: AST.MustacheStatement, locals: string[][]): boolean { | ||
let { path } = node; | ||
return isPath(path) && isIllegalName(path, locals) && hasArguments(node); | ||
} | ||
|
||
function isPath(node: AST.PathExpression | AST.Literal): node is AST.PathExpression { | ||
return node.type === 'PathExpression'; | ||
} | ||
|
||
function isIllegalName(node: AST.PathExpression, locals: string[][]): boolean { | ||
return isThisPath(node) || isDotPath(node) || isLocalVariable(node, locals); | ||
} | ||
|
||
function isThisPath(node: AST.PathExpression): boolean { | ||
return node.this === true; | ||
} | ||
|
||
function isDotPath(node: AST.PathExpression): boolean { | ||
return node.parts.length > 1; | ||
} | ||
|
||
function isLocalVariable(node: AST.PathExpression, locals: string[][]): boolean { | ||
return !node.this && hasLocalVariable(node.parts[0], locals); | ||
} | ||
|
||
function hasLocalVariable(name: string, locals: string[][]): boolean { | ||
return locals.some(names => names.indexOf(name) !== -1); | ||
} | ||
|
||
function hasArguments(node: AST.MustacheStatement): boolean { | ||
return node.params.length > 0 || node.hash.pairs.length > 0; | ||
} | ||
|
||
function isBlockInvocation(node: AST.BlockStatement, locals: string[][]): boolean { | ||
return isIllegalName(node.path, locals); | ||
} | ||
|
||
let wrapInAssertion: ( | ||
moduleName: string, | ||
node: AST.PathExpression, | ||
builder: Builders | ||
) => AST.Expression; | ||
|
||
if (DEBUG) { | ||
wrapInAssertion = (moduleName, node, b) => { | ||
let error = b.string( | ||
`expected \`${ | ||
node.original | ||
}\` to be a contextual component but found a string. Did you mean \`(component ${ | ||
node.original | ||
})\`? ${calculateLocationDisplay(moduleName, node.loc)}` | ||
); | ||
|
||
return b.sexpr( | ||
b.path('-assert-implicit-component-helper-argument'), | ||
[node, error], | ||
b.hash(), | ||
node.loc | ||
); | ||
}; | ||
} else { | ||
wrapInAssertion = (_, node) => node; | ||
} | ||
|
||
function wrapInComponent( | ||
moduleName: string, | ||
node: AST.MustacheStatement | AST.BlockStatement, | ||
b: Builders | ||
) { | ||
let component = wrapInAssertion(moduleName, node.path as AST.PathExpression, b); | ||
node.path = b.path('component'); | ||
node.params.unshift(component); | ||
} |
Oops, something went wrong.