Merge pull request #15848 from rwjblue/ensure-helpers-are-consistent

[BUGFIX beta] Ensure helpers have a consistent API.

packages/ember-application/tests/system/dependency_injection/default_resolver_test.js

@@ -98,7 +98,7 @@ moduleFor('Ember.Application Dependency Injection - Integration - default resolv
 
     let lookedUpShorthandHelper = this.applicationInstance.factoryFor('helper:shorthand').class;
 
-    assert.ok(lookedUpShorthandHelper.isHelperInstance, 'shorthand helper isHelper');
+    assert.ok(lookedUpShorthandHelper.isHelperFactory, 'shorthand helper isHelper');
 
     let lookedUpHelper = this.applicationInstance.factoryFor('helper:complete').class;
 
@@ -115,7 +115,7 @@ moduleFor('Ember.Application Dependency Injection - Integration - default resolv
 
     let lookedUpShorthandHelper = this.applicationInstance.factoryFor('helper:shorthand').class;
 
-    assert.ok(lookedUpShorthandHelper.isHelperInstance, 'shorthand helper isHelper');
+    assert.ok(lookedUpShorthandHelper.isHelperFactory, 'shorthand helper isHelper');
 
     let lookedUpHelper = this.applicationInstance.factoryFor('helper:complete').class;
 

packages/ember-glimmer/lib/environment.ts

@@ -1,8 +1,10 @@
 import {
   Reference,
+  PathReference,
 } from '@glimmer/reference';
 import {
   AttributeManager,
+  CapturedArguments,
   CompilableLayout,
   CompiledDynamicProgram,
   compileLayout,
@@ -15,6 +17,7 @@ import {
   PrimitiveReference,
   PartialDefinition,
   Simple,
+  VM
 } from '@glimmer/runtime';
 import {
   Destroyable, Opaque,
@@ -285,13 +288,18 @@ export default class Environment extends GlimmerEnvironment {
     let helperFactory = owner.factoryFor(`helper:${name}`, options) || owner.factoryFor(`helper:${name}`);
 
     // TODO: try to unify this into a consistent protocol to avoid wasteful closure allocations
-    if (helperFactory.class.isHelperInstance) {
-      return (_vm, args) => SimpleHelperReference.create(helperFactory.class.compute, args.capture());
+    let HelperReference: {
+      create: (HelperFactory: any, vm: VM, args: CapturedArguments ) => PathReference<Opaque>;
+    };
+    if (helperFactory.class.isSimpleHelperFactory) {
+      HelperReference = SimpleHelperReference;
     } else if (helperFactory.class.isHelperFactory) {
-      return (vm, args) => ClassBasedHelperReference.create(helperFactory, vm, args.capture());
+      HelperReference = ClassBasedHelperReference;
     } else {
       throw new Error(`${name} is not a helper`);
     }
+
+    return (vm, args) => HelperReference.create(helperFactory, vm, args.capture());
   }
 
   hasModifier(name: string) {

packages/ember-glimmer/lib/helper.ts

@@ -104,6 +104,29 @@ Helper.reopenClass({
   isHelperFactory: true,
 });
 
+export interface HelperInstance {
+  isHelperInstance: true;
+  compute: Function;
+}
+
+export class SimpleHelperFactory {
+  isHelperFactory = true;
+  isSimpleHelperFactory = true;
+
+  private instance: HelperInstance;
+
+  constructor(compute: (positionalValue: any[], namedValue: any[]) => any) {
+    this.instance = {
+      isHelperInstance: true,
+      compute,
+    };
+  }
+
+  create(): HelperInstance {
+    return this.instance;
+  }
+}
+
 /**
   In many cases, the ceremony of a full `Helper` class is not required.
   The `helper` method create pure-function helpers without instances. For
@@ -127,10 +150,7 @@ Helper.reopenClass({
   @since 1.13.0
 */
 export function helper(helperFn: (params: any[], hash?: any) => string) {
-  return {
-    isHelperInstance: true,
-    compute: helperFn,
-  };
+  return new SimpleHelperFactory(helperFn);
 }
 
 export default Helper;

packages/ember-glimmer/lib/utils/references.ts

@@ -32,7 +32,10 @@ import {
   EMBER_GLIMMER_DETECT_BACKTRACKING_RERENDER,
   MANDATORY_SETTER,
 } from 'ember/features';
-import { RECOMPUTE_TAG } from '../helper';
+import {
+  RECOMPUTE_TAG,
+  SimpleHelperFactory,
+} from '../helper';
 import emberToBool from './to-bool';
 
 export const UPDATE = symbol('UPDATE');
@@ -329,7 +332,9 @@ export class SimpleHelperReference extends CachedReference {
   public helper: (positionalValue: any, namedValue: any) => any;
   public args: any;
 
-  static create(helper: (positionalValue: any, namedValue: any) => any, args: CapturedArguments) {
+  static create(Helper: SimpleHelperFactory, _vm: VM, args: CapturedArguments) {
+    let helper = Helper.create();
+
     if (isConst(args)) {
       let { positional, named } = args;
 
@@ -341,15 +346,15 @@ export class SimpleHelperReference extends CachedReference {
         maybeFreeze(namedValue);
       }
 
-      let result = helper(positionalValue, namedValue);
+      let result = helper.compute(positionalValue, namedValue);
 
       if (typeof result === 'object' && result !== null || typeof result === 'function') {
         return new RootReference(result);
       } else {
         return PrimitiveReference.create(result);
       }
     } else {
-      return new SimpleHelperReference(helper, args);
+      return new SimpleHelperReference(helper.compute, args);
     }
   }
 

packages/ember-glimmer/tests/integration/helpers/custom-helper-test.js

@@ -564,6 +564,32 @@ moduleFor('Helpers test: custom helpers', class extends RenderingTest {
 
     equal(destroyCount, 1, 'destroy is called after a view is destroyed');
   }
+
+  ['@test simple helper can be invoked manually via `owner.factoryFor(...).create().compute()'](assert) {
+    this.registerHelper('some-helper', () => {
+      assert.ok(true, 'some-helper helper invoked');
+      return 'lolol';
+    });
+
+    let instance = this.owner.factoryFor('helper:some-helper').create();
+
+    assert.equal(typeof instance.compute, 'function', 'expected instance.compute to be present');
+    assert.equal(instance.compute(), 'lolol', 'can invoke `.compute`');
+  }
+
+  ['@test class-based helper can be invoked manually via `owner.factoryFor(...).create().compute()']() {
+    this.registerHelper('some-helper', {
+      compute() {
+        assert.ok(true, 'some-helper helper invoked');
+        return 'lolol';
+      }
+    });
+
+    let instance = this.owner.factoryFor('helper:some-helper').create();
+
+    assert.equal(typeof instance.compute, 'function', 'expected instance.compute to be present');
+    assert.equal(instance.compute(), 'lolol', 'can invoke `.compute`');
+  }
 });
 
 // these feature detects prevent errors in these tests