Make the helper keyword handling less strict

The current implementation throws an error if you try to curry helpers using the helper keyword. This reduces the strictness of the resolver since Ember itself catches the cases were users provide unsupported dynamic values to the helper keyword.
embroider-build · Mar 5, 2022 · 4ca8705 · 4ca8705
1 parent 2a44b73
commit 4ca8705
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 26 deletions.
diff --git a/packages/compat/src/resolver-transform.ts b/packages/compat/src/resolver-transform.ts
@@ -329,15 +329,12 @@ function handleComponentHelper(
   resolver.resolveComponentHelper(locator, moduleName, param.loc, impliedBecause);
 }
 
-function handleDynamicHelper(param: ASTv1.Node, resolver: Resolver, moduleName: string): void {
-  switch (param.type) {
-    case 'StringLiteral':
-      resolver.resolveDynamicHelper({ type: 'literal', path: param.value }, moduleName, param.loc);
-      break;
-    case 'TextNode':
-      resolver.resolveDynamicHelper({ type: 'literal', path: param.chars }, moduleName, param.loc);
-      break;
-    default:
-      resolver.resolveDynamicHelper({ type: 'other' }, moduleName, param.loc);
+function handleDynamicHelper(param: ASTv1.Expression, resolver: Resolver, moduleName: string): void {
+  // We only need to handle StringLiterals since Ember already throws an error if unsupported values
+  // are passed to the helper keyword.
+  // If a helper reference is passed in we don't need to do anything since it's either the result of a previous
+  // helper keyword invocation, or a helper reference that was imported somewhere.
+  if (param.type === 'StringLiteral') {
+    resolver.resolveDynamicHelper({ type: 'literal', path: param.value }, moduleName, param.loc);
   }
 }
diff --git a/packages/compat/tests/resolver.test.ts b/packages/compat/tests/resolver.test.ts
@@ -519,7 +519,7 @@ describe('compat-resolver', function () {
       },
     ]);
   });
-  test('string literal passed to `helper` helper in content position', function () {
+  test('string literal passed to "helper" keyword in content position', function () {
     let findDependencies = configure({
       staticHelpers: true,
     });
@@ -546,7 +546,7 @@ describe('compat-resolver', function () {
       )
     ).toEqual([]);
   });
-  test('built-in helpers are ignored when used with the "helper" helper', function () {
+  test('built-in helpers are ignored when used with the "helper" keyword', function () {
     let findDependencies = configure({
       staticHelpers: true,
     });
@@ -662,7 +662,7 @@ describe('compat-resolver', function () {
       },
     ]);
   });
-  test('string literal passed to "helper" helper in helper position', function () {
+  test('string literal passed to "helper" keyword in helper position', function () {
     let findDependencies = configure({ staticHelpers: true });
     givenFile('helpers/hello-world.js');
     expect(
@@ -681,14 +681,35 @@ describe('compat-resolver', function () {
       },
     ]);
   });
+  test('helper currying using the "helper" keyword', function () {
+    let findDependencies = configure({ staticHelpers: true });
+    givenFile('helpers/hello-world.js');
+    expect(
+      findDependencies(
+        'templates/application.hbs',
+        `
+        {{#let (helper "hello-world" name="World") as |hello|}}
+          {{#let (helper hello name="Tomster") as |helloTomster|}}
+            {{helloTomster name="Zoey"}}
+          {{/let}}
+        {{/let}}
+        `
+      )
+    ).toEqual([
+      {
+        path: '../helpers/hello-world.js',
+        runtimeName: 'the-app/helpers/hello-world',
+      },
+    ]);
+  });
   test('string literal passed to component helper fails to resolve', function () {
     let findDependencies = configure({ staticComponents: true });
     givenFile('components/my-thing.js');
     expect(() => {
       findDependencies('templates/application.hbs', `{{my-thing header=(component "hello-world") }}`);
     }).toThrow(new RegExp(`Missing component: hello-world in templates/application.hbs`));
   });
-  test('string literal passed to "helper" helper fails to resolve', function () {
+  test('string literal passed to "helper" keyword fails to resolve', function () {
     let findDependencies = configure({ staticHelpers: true });
     expect(() => {
       findDependencies('templates/application.hbs', `{{helper "hello-world"}}`);
@@ -699,7 +720,7 @@ describe('compat-resolver', function () {
     givenFile('components/my-thing.js');
     expect(findDependencies('templates/application.hbs', `{{my-thing header=(component "hello-world") }}`)).toEqual([]);
   });
-  test('string literal passed to "helper" helper fails to resolve when staticHelpers is off', function () {
+  test('string literal passed to "helper" keyword fails to resolve when staticHelpers is off', function () {
     let findDependencies = configure({ staticHelpers: false });
     expect(findDependencies('templates/application.hbs', `{{helper "hello-world"}}`)).toEqual([]);
   });
@@ -1764,18 +1785,9 @@ describe('compat-resolver', function () {
     );
   });
 
-  test('rejects arbitrary expression in "helper" helper', function () {
+  test('ignores any non-string-literal in "helper" keyword', function () {
     let findDependencies = configure({ staticHelpers: true });
-    expect(() => findDependencies('templates/application.hbs', `{{helper (some-helper this.which) }}`)).toThrow(
-      `Unsafe dynamic helper: cannot statically analyze this expression`
-    );
-  });
-
-  test('rejects any non-string-literal in "helper" helper', function () {
-    let findDependencies = configure({ staticHelpers: true });
-    expect(() => findDependencies('templates/application.hbs', `{{helper this.which }}`)).toThrow(
-      `Unsafe dynamic helper: cannot statically analyze this expression`
-    );
+    expect(findDependencies('templates/application.hbs', `{{helper this.which}}`)).toEqual([]);
   });
 
   test('trusts inline ensure-safe-component helper', function () {