Merge pull request #19487 from emberjs/dynamic-helper-modifier-resolu…

…tion

[BUGFIX beta] Allow passing a string literal to {{helper}} and {{modifier}}

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;
+  });
+});

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}")`);
+});

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,

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(
+      //   '[<div {{modifier "replace"}}>Nope</div>][<div {{modifier (modifier "replace") "wow"}}>Nope</div>]'
+      // );
+      this.render(
+        '[<div {{(modifier "replace")}}>Nope</div>][<div {{(modifier "replace") "wow"}}>Nope</div>]'
+      );
+      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
+              // '<div {{modifier this.name}}>Nope</div>',
+              '<div {{(modifier this.name)}}>Nope</div>',
+              { 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));
 

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);
 

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)
 );