From 397d6b990fdf2e46bdc97a4424419706184e4e0a Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Tue, 30 Mar 2021 02:05:06 -0700 Subject: [PATCH] [BUGFIX beta] Allow passing a string literal to {{helper}} and {{modifier}} As per [RFC #432](https://github.com/emberjs/rfcs/blob/master/text/0432-contextual-helpers.md#invoking-contextual-modifiers) However, dynamic string values are currently disallowed per earlier framework team weekly meeting discussion to not make it harder for Embroider to heuristically analyze the dynamic usages. Eventually we will want to do the same for components as well, and this AST transform would work there too. --- .../helpers/-disallow-dynamic-resolution.ts | 57 +++++ .../glimmer/lib/helpers/-resolve.ts | 39 ++++ .../@ember/-internals/glimmer/lib/resolver.ts | 17 ++ .../custom-modifier-manager-test.js | 39 ++++ .../integration/helpers/custom-helper-test.js | 21 ++ .../lib/plugins/index.ts | 5 +- .../lib/plugins/transform-resolutions.ts | 198 ++++++++++++++++++ .../lib/plugins/utils.ts | 8 +- 8 files changed, 381 insertions(+), 3 deletions(-) create mode 100644 packages/@ember/-internals/glimmer/lib/helpers/-disallow-dynamic-resolution.ts create mode 100644 packages/@ember/-internals/glimmer/lib/helpers/-resolve.ts create mode 100644 packages/ember-template-compiler/lib/plugins/transform-resolutions.ts diff --git a/packages/@ember/-internals/glimmer/lib/helpers/-disallow-dynamic-resolution.ts b/packages/@ember/-internals/glimmer/lib/helpers/-disallow-dynamic-resolution.ts new file mode 100644 index 00000000000..7d761c5c768 --- /dev/null +++ b/packages/@ember/-internals/glimmer/lib/helpers/-disallow-dynamic-resolution.ts @@ -0,0 +1,57 @@ +/** +@module ember +*/ +import { assert } from '@ember/debug'; +import { CapturedArguments } from '@glimmer/interfaces'; +import { createComputeRef, valueForRef } from '@glimmer/reference'; +import { internalHelper } from './internal-helper'; + +export default internalHelper(({ positional, named }: CapturedArguments) => { + assert( + `[BUG] wrong number of positional arguments, expecting 1, got ${positional.length}`, + positional.length === 1 + ); + + let nameOrValueRef = positional[0]; + + assert(`[BUG] expecting \`type\` named argument`, 'type' in named); + assert(`[BUG] expecting \`loc\` named argument`, 'loc' in named); + assert(`[BUG] expecting \`original\` named argument`, 'original' in named); + + let typeRef = named.type; + let locRef = named.loc; + let originalRef = named.original; + + // Bug: why do these fail? + // assert('[BUG] expecting a string literal for the `type` argument', isConstRef(typeRef)); + // assert('[BUG] expecting a string literal for the `loc` argument', isConstRef(locRef)); + // assert('[BUG] expecting a string literal for the `original` argument', isConstRef(originalRef)); + + const type = valueForRef(typeRef); + const loc = valueForRef(locRef); + const original = valueForRef(originalRef); + + assert('[BUG] expecting a string literal for the `type` argument', typeof type === 'string'); + assert('[BUG] expecting a string literal for the `loc` argument', typeof loc === 'string'); + assert( + '[BUG] expecting a string literal for the `original` argument', + typeof original === 'string' + ); + + return createComputeRef(() => { + let nameOrValue = valueForRef(nameOrValueRef); + + assert( + `Passing a dynamic string to the \`(${type})\` keyword is disallowed. ` + + `(You specified \`(${type} ${original})\` and \`${original}\` evaluated into "${nameOrValue}".) ` + + `This ensures we can statically analyze the template and determine which ${type}s are used. ` + + `If the ${type} name is always the same, use a string literal instead, i.e. \`(${type} "${nameOrValue}")\`. ` + + `Otherwise, import the ${type}s into JavaScript and pass them to the ${type} keyword. ` + + 'See https://github.com/emberjs/rfcs/blob/master/text/0496-handlebars-strict-mode.md#4-no-dynamic-resolution for details. ' + + loc, + typeof nameOrValue !== 'string' + ); + + return nameOrValue; + }); +}); diff --git a/packages/@ember/-internals/glimmer/lib/helpers/-resolve.ts b/packages/@ember/-internals/glimmer/lib/helpers/-resolve.ts new file mode 100644 index 00000000000..34eda61c3c2 --- /dev/null +++ b/packages/@ember/-internals/glimmer/lib/helpers/-resolve.ts @@ -0,0 +1,39 @@ +/** +@module ember +*/ +import { Owner } from '@ember/-internals/owner'; +import { assert } from '@ember/debug'; +import { DEBUG } from '@glimmer/env'; +import { CapturedArguments } from '@glimmer/interfaces'; +import { createConstRef, isConstRef, valueForRef } from '@glimmer/reference'; +import { internalHelper } from './internal-helper'; + +export default internalHelper(({ positional }: CapturedArguments, owner: Owner | undefined) => { + // why is this allowed to be undefined in the first place? + assert('[BUG] missing owner', owner); + + assert( + `[BUG] wrong number of positional arguments, expecting 1, got ${positional.length}`, + positional.length === 1 + ); + + let fullNameRef = positional[0]; + + assert('[BUG] expecting a string literal as argument', isConstRef(fullNameRef)); + + let fullName = valueForRef(fullNameRef); + + assert('[BUG] expecting a string literal as argument', typeof fullName === 'string'); + assert('[BUG] expecting a valid full name', fullName.split(':').length === 2); + + if (DEBUG) { + let [type, name] = fullName.split(':'); + + assert( + `Attempted to invoke \`(-resolve "${fullName}")\`, but ${name} was not a valid ${type} name.`, + owner.hasRegistration(fullName) + ); + } + + return createConstRef(owner.factoryFor(fullName)?.class, `(-resolve "${fullName}")`); +}); diff --git a/packages/@ember/-internals/glimmer/lib/resolver.ts b/packages/@ember/-internals/glimmer/lib/resolver.ts index 8c87ec67c64..c20abe47c5f 100644 --- a/packages/@ember/-internals/glimmer/lib/resolver.ts +++ b/packages/@ember/-internals/glimmer/lib/resolver.ts @@ -42,8 +42,10 @@ import { isClassicHelper, SimpleHelper, } from './helper'; +import { default as disallowDynamicResolution } from './helpers/-disallow-dynamic-resolution'; import { default as inElementNullCheckHelper } from './helpers/-in-element-null-check'; import { default as normalizeClassHelper } from './helpers/-normalize-class'; +import { default as resolve } from './helpers/-resolve'; import { default as trackArray } from './helpers/-track-array'; import { default as action } from './helpers/action'; import { default as eachIn } from './helpers/each-in'; @@ -181,12 +183,27 @@ const BUILTIN_KEYWORD_HELPERS = { '-hash': hash, '-each-in': eachIn, '-normalize-class': normalizeClassHelper, + '-resolve': resolve, '-track-array': trackArray, '-mount': mountHelper, '-outlet': outletHelper, '-in-el-null': inElementNullCheckHelper, }; +if (DEBUG) { + BUILTIN_KEYWORD_HELPERS['-disallow-dynamic-resolution'] = disallowDynamicResolution; +} else { + // Bug: this may be a quirk of our test setup? + // In prod builds, this is a no-op helper and is unused in practice. We shouldn't need + // to add it at all, but the current test build doesn't produce a "prod compiler", so + // we ended up running the debug-build for the template compliler in prod tests. Once + // that is fixed, this can be removed. For now, this allows the test to work and does + // not really harm anything, since it's just a no-op pass-through helper and the bytes + // has to be included anyway. In the future, perhaps we can avoid the latter by using + // `import(...)`? + BUILTIN_KEYWORD_HELPERS['-disallow-dynamic-resolution'] = disallowDynamicResolution; +} + const BUILTIN_HELPERS = { ...BUILTIN_KEYWORD_HELPERS, array, 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 index dd92643ce33..508dfc567ed 100644 --- a/packages/@ember/-internals/glimmer/tests/integration/custom-modifier-manager-test.js +++ b/packages/@ember/-internals/glimmer/tests/integration/custom-modifier-manager-test.js @@ -581,6 +581,45 @@ moduleFor( assert.equal(updateCount, 1); } + '@feature(EMBER_DYNAMIC_HELPERS_AND_MODIFIERS) Can resolve a modifier'() { + this.registerModifier( + 'replace', + defineSimpleModifier((element, [text]) => (element.innerHTML = text ?? 'Hello, world!')) + ); + + // BUG: this should work according to the RFC + // this.render( + // '[
Nope
][
Nope
]' + // ); + this.render( + '[
Nope
][
Nope
]' + ); + this.assertText('[Hello, world!][wow]'); + this.assertStableRerender(); + } + + '@feature(EMBER_DYNAMIC_HELPERS_AND_MODIFIERS) Cannot dynamically resolve a modifier'(assert) { + this.registerModifier( + 'replace', + defineSimpleModifier((element) => (element.innerHTML = 'Hello, world!')) + ); + + if (DEBUG) { + expectAssertion( + () => + this.render( + // BUG: this should work according to the RFC + // '
Nope
', + '
Nope
', + { name: 'replace' } + ), + /Passing a dynamic string to the `\(modifier\)` keyword is disallowed\./ + ); + } else { + assert.expect(0); + } + } + '@feature(EMBER_DYNAMIC_HELPERS_AND_MODIFIERS) Can be curried'() { let val = defineSimpleModifier((element, [text]) => (element.innerHTML = text)); 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 index 08d55ef94b9..ba4bb014444 100644 --- a/packages/@ember/-internals/glimmer/tests/integration/helpers/custom-helper-test.js +++ b/packages/@ember/-internals/glimmer/tests/integration/helpers/custom-helper-test.js @@ -780,6 +780,27 @@ moduleFor( }, expectedMessage); } + '@feature(EMBER_DYNAMIC_HELPERS_AND_MODIFIERS) Can resolve a helper'() { + this.registerHelper('hello-world', ([text]) => text ?? 'Hello, world!'); + + this.render('[{{helper "hello-world"}}][{{helper (helper "hello-world") "wow"}}]'); + this.assertText('[Hello, world!][wow]'); + this.assertStableRerender(); + } + + '@feature(EMBER_DYNAMIC_HELPERS_AND_MODIFIERS) Cannot dynamically resolve a helper'(assert) { + this.registerHelper('hello-world', () => 'Hello, world!'); + + if (DEBUG) { + expectAssertion( + () => this.render('{{helper this.name}}', { name: 'hello-world' }), + /Passing a dynamic string to the `\(helper\)` keyword is disallowed\./ + ); + } else { + assert.expect(0); + } + } + '@feature(EMBER_DYNAMIC_HELPERS_AND_MODIFIERS) Can use a curried dynamic helper'() { let val = defineSimpleHelper((value) => value); diff --git a/packages/ember-template-compiler/lib/plugins/index.ts b/packages/ember-template-compiler/lib/plugins/index.ts index 4b8ac4fe831..2efa104176a 100644 --- a/packages/ember-template-compiler/lib/plugins/index.ts +++ b/packages/ember-template-compiler/lib/plugins/index.ts @@ -14,6 +14,7 @@ import TransformInElement from './transform-in-element'; import TransformLinkTo from './transform-link-to'; import TransformOldClassBindingSyntax from './transform-old-class-binding-syntax'; import TransformQuotedBindingsIntoJustBindings from './transform-quoted-bindings-into-just-bindings'; +import TransformResolutions from './transform-resolutions'; import TransformWrapMountAndOutlet from './transform-wrap-mount-and-outlet'; import { EMBER_DYNAMIC_HELPERS_AND_MODIFIERS, EMBER_NAMED_BLOCKS } from '@ember/canary-features'; @@ -38,7 +39,9 @@ export const RESOLUTION_MODE_TRANSFORMS = Object.freeze( DeprecateWith, SEND_ACTION ? DeprecateSendAction : null, !EMBER_NAMED_BLOCKS ? AssertAgainstNamedBlocks : null, - !EMBER_DYNAMIC_HELPERS_AND_MODIFIERS ? AssertAgainstDynamicHelpersModifiers : null, + EMBER_DYNAMIC_HELPERS_AND_MODIFIERS + ? TransformResolutions + : AssertAgainstDynamicHelpersModifiers, ].filter(notNull) ); diff --git a/packages/ember-template-compiler/lib/plugins/transform-resolutions.ts b/packages/ember-template-compiler/lib/plugins/transform-resolutions.ts new file mode 100644 index 00000000000..14f3849f1ce --- /dev/null +++ b/packages/ember-template-compiler/lib/plugins/transform-resolutions.ts @@ -0,0 +1,198 @@ +import { assert } from '@ember/debug'; +import { DEBUG } from '@glimmer/env'; +import { AST, ASTPlugin, print } from '@glimmer/syntax'; +import calculateLocationDisplay from '../system/calculate-location-display'; +import { EmberASTPluginEnvironment } from '../types'; +import { isPath, isStringLiteral, trackLocals } from './utils'; + +/** + @module ember +*/ + +/** + A Glimmer2 AST transformation that replaces all instances of + + ```handlebars + {{helper "..." ...}} + ``` + + with + + ```handlebars + {{helper (-resolve "helper:...") ...}} + ``` + + and + + ```handlebars + {{helper ... ...}} + ``` + + with + + ```handlebars + {{helper (-disallow-dynamic-resolution ...) ...}} + ``` + + and + + ```handlebars + {{modifier "..." ...}} + ``` + + with + + ```handlebars + {{modifier (-resolve "modifier:...") ...}} + ``` + and + + ```handlebars + {{modifier ... ...}} + ``` + + with + + ```handlebars + {{modifier (-disallow-dynamic-resolution ...) ...}} + ``` + + @private + @class TransformResolutions +*/ + +const TARGETS = Object.freeze(['helper', 'modifier']); + +export default function transformResolutions(env: EmberASTPluginEnvironment): ASTPlugin { + let { builders: b } = env.syntax; + let { moduleName } = env.meta; + let { hasLocal, node: tracker } = trackLocals(); + let seen: Set | undefined; + + return { + name: 'transform-resolutions', + + visitor: { + Template: { + enter() { + seen = new Set(); + }, + + exit() { + seen = undefined; + }, + }, + + Block: tracker, + + ElementNode: { + keys: { + children: tracker, + }, + }, + + MustacheStatement(node: AST.MustacheStatement): AST.Node | void { + assert('[BUG] seen set should be available', seen); + + if (seen.has(node)) { + return; + } + + if ( + isPath(node.path) && + !isLocalVariable(node.path, hasLocal) && + TARGETS.indexOf(node.path.original) !== -1 + ) { + let result = b.mustache( + node.path, + transformParams(b, node.params, node.path.original, moduleName, node.loc), + node.hash, + node.trusting, + node.loc, + node.strip + ); + + // Avoid double/infinite-processing + seen.add(result); + + return result; + } + }, + SubExpression(node: AST.SubExpression): AST.Node | void { + assert('[BUG] seen set should be available', seen); + + if (seen.has(node)) { + return; + } + + if ( + isPath(node.path) && + !isLocalVariable(node.path, hasLocal) && + TARGETS.indexOf(node.path.original) !== -1 + ) { + let result = b.sexpr( + node.path, + transformParams(b, node.params, node.path.original, moduleName, node.loc), + node.hash, + node.loc + ); + + // Avoid double/infinite-processing + seen.add(result); + + return result; + } + }, + }, + }; +} + +function isLocalVariable(node: AST.PathExpression, hasLocal: (k: string) => boolean): boolean { + return !node.this && node.parts.length === 1 && hasLocal(node.parts[0]); +} + +function transformParams( + b: EmberASTPluginEnvironment['syntax']['builders'], + params: AST.Expression[], + type: string, + moduleName: string, + loc: AST.SourceLocation | undefined +): AST.Expression[] { + let [first, ...rest] = params; + + assert( + `The ${type} keyword requires at least one positional arguments ${calculateLocationDisplay( + moduleName, + loc + )}`, + first + ); + + if (isStringLiteral(first)) { + return [ + b.sexpr( + b.path('-resolve', first.loc), + [b.string(`${type}:${first.value}`)], + undefined, + first.loc + ), + ...rest, + ]; + } else if (DEBUG) { + return [ + b.sexpr( + b.path('-disallow-dynamic-resolution', first.loc), + [first], + b.hash([ + b.pair('type', b.string(type), first.loc), + b.pair('loc', b.string(calculateLocationDisplay(moduleName, loc)), first.loc), + b.pair('original', b.string(print(first))), + ]), + first.loc + ), + ...rest, + ]; + } else { + return params; + } +} diff --git a/packages/ember-template-compiler/lib/plugins/utils.ts b/packages/ember-template-compiler/lib/plugins/utils.ts index fa310f00e31..a2e42b39ca0 100644 --- a/packages/ember-template-compiler/lib/plugins/utils.ts +++ b/packages/ember-template-compiler/lib/plugins/utils.ts @@ -8,18 +8,22 @@ export function isSubExpression(node: AST.Node): node is AST.SubExpression { return node.type === 'SubExpression'; } +export function isStringLiteral(node: AST.Expression): node is AST.StringLiteral { + return node.type === 'StringLiteral'; +} + export function trackLocals() { let locals = new Map(); let node = { - enter(node: AST.Program | AST.ElementNode) { + enter(node: AST.Program | AST.Block | AST.ElementNode) { for (let param of node.blockParams) { let value = locals.get(param) || 0; locals.set(param, value + 1); } }, - exit(node: AST.Program | AST.ElementNode) { + exit(node: AST.Program | AST.Block | AST.ElementNode) { for (let param of node.blockParams) { let value = locals.get(param) - 1;