Merge pull request #16600 from emberjs/fix-simple-helper

[BUGFIX release] Fix SimpleHelper memory leak
emberjs · May 2, 2018 · 3d91206 · 3d91206
2 parents da16244 + 86d7f50
commit 3d91206
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 35 deletions.
diff --git a/packages/ember-glimmer/lib/helper.ts b/packages/ember-glimmer/lib/helper.ts
@@ -12,39 +12,33 @@ export const RECOMPUTE_TAG = symbol('RECOMPUTE_TAG');
 
 export type HelperFunction = (positional: Opaque[], named: Dict<Opaque>) => Opaque;
 
-export type SimpleHelperFactory = Factory<SimpleHelper, SimpleHelper>;
-export type ClassHelperFactory = Factory<HelperInstance, HelperStatic>;
+export type SimpleHelperFactory = Factory<SimpleHelper, HelperFactory<SimpleHelper>>;
+export type ClassHelperFactory = Factory<HelperInstance, HelperFactory<HelperInstance>>;
 
-export type HelperFactory = SimpleHelperFactory | ClassHelperFactory;
-
-export interface SimpleHelper {
+export interface HelperFactory<T> {
   isHelperFactory: true;
-  isSimpleHelper: true;
-
-  create(): SimpleHelper;
-  compute: HelperFunction;
+  create(): T;
 }
 
-export interface HelperStatic {
-  isHelperFactory: true;
-  isSimpleHelper: false;
-
-  create(): HelperInstance;
+export interface HelperInstance {
+  compute(positional: Opaque[], named: Dict<Opaque>): Opaque;
+  destroy(): void;
 }
 
-export interface HelperInstance {
+export interface SimpleHelper {
   compute: HelperFunction;
-  destroy(): void;
 }
 
-export function isHelperFactory(helper: any | undefined | null): helper is HelperFactory {
+export function isHelperFactory(
+  helper: any | undefined | null
+): helper is SimpleHelperFactory | ClassHelperFactory {
   return (
     typeof helper === 'object' && helper !== null && helper.class && helper.class.isHelperFactory
   );
 }
 
-export function isSimpleHelper(helper: HelperFactory): helper is SimpleHelperFactory {
-  return helper.class.isSimpleHelper;
+export function isSimpleHelper(helper: SimpleHelper | HelperInstance): helper is SimpleHelper {
+  return (helper as any).destroy === undefined;
 }
 
 /**
@@ -135,19 +129,18 @@ let Helper = FrameworkObject.extend({
   */
 });
 
-Helper.reopenClass({
-  isHelperFactory: true,
-  isSimpleHelper: false,
-});
+Helper.isHelperFactory = true;
 
-class Wrapper implements SimpleHelper {
+class Wrapper implements HelperFactory<SimpleHelper> {
   isHelperFactory: true = true;
-  isSimpleHelper: true = true;
 
   constructor(public compute: HelperFunction) {}
 
   create() {
-    return this;
+    // needs new instance or will leak containers
+    return {
+      compute: this.compute,
+    };
   }
 }
 
@@ -173,8 +166,8 @@ class Wrapper implements SimpleHelper {
   @public
   @since 1.13.0
 */
-export function helper(helperFn: HelperFunction): SimpleHelper {
+export function helper(helperFn: HelperFunction): HelperFactory<SimpleHelper> {
   return new Wrapper(helperFn);
 }
 
-export default Helper as HelperStatic;
+export default Helper as HelperFactory<HelperInstance>;
diff --git a/packages/ember-glimmer/lib/resolver.ts b/packages/ember-glimmer/lib/resolver.ts
@@ -244,15 +244,11 @@ export default class RuntimeResolver implements IRuntimeResolver<OwnedTemplateMe
       return null;
     }
 
-    if (isSimpleHelper(factory)) {
-      const helper = factory.create().compute;
-      return (_vm, args) => {
-        return SimpleHelperReference.create(helper, args.capture());
-      };
-    }
-
     return (vm, args) => {
       const helper = factory.create();
+      if (isSimpleHelper(helper)) {
+        return new SimpleHelperReference(helper.compute, args.capture());
+      }
       vm.newDestroyable(helper);
       return ClassBasedHelperReference.create(helper, args.capture());
     };