Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX release] Fix SimpleHelper memory leak #16600

Merged
merged 1 commit into from
May 2, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 21 additions & 28 deletions packages/ember-glimmer/lib/helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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,
};
}
}

Expand All @@ -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>;
10 changes: 3 additions & 7 deletions packages/ember-glimmer/lib/resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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());
};
Expand Down