Skip to content

Commit

Permalink
Revert changing helpers to return References
Browse files Browse the repository at this point in the history
Helpers are required to return `PathReference`s`, because their return
values can be later used in lookup positions, e.g.

```
{{#with (my-helper) as |foo|}}
  {{foo.bar}} <--- referenceFromMyHelper.get('bar')
{{/with}}
```

See emberjs/ember.js#13173 (comment) for more details.

We were previously using `Function.prototype.call` to invoke helpers,
which is typed to be `call(thisArg: any, args: any[]): any` which caused
us to not get a TypeError from the change (because the return value is
`any`). This commit fixed that as well.
  • Loading branch information
Godhuda committed Apr 20, 2016
1 parent 6dfc32a commit 8001311
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 13 deletions.
3 changes: 2 additions & 1 deletion packages/glimmer-runtime/lib/compiled/expressions/helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ export default class CompiledHelper extends CompiledExpression<Opaque> {
}

evaluate(vm: VM): PathReference<Opaque> {
return this.helper.call(undefined, this.args.evaluate(vm));
let { helper } = this;
return helper(this.args.evaluate(vm));
}

toJSON(): string {
Expand Down
2 changes: 1 addition & 1 deletion packages/glimmer-runtime/lib/environment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ type PositionalArguments = Opaque[];
type KeywordArguments = Dict<Opaque>;

export interface Helper {
(args: EvaluatedArgs): Reference<Opaque>;
(args: EvaluatedArgs): PathReference<Opaque>;
}

export interface ParsedStatement {
Expand Down
20 changes: 10 additions & 10 deletions packages/glimmer-runtime/tests/updating-test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { EvaluatedArgs, Template, RenderResult, SafeString } from "glimmer-runtime";
import { EvaluatedArgs, Template, RenderResult, SafeString, ValueReference } from "glimmer-runtime";
import { TestEnvironment, TestDynamicScope, equalTokens, stripTight } from "glimmer-test-helpers";
import { PathReference, ConstReference } from "glimmer-reference";
import { PathReference } from "glimmer-reference";
import { UpdatableReference } from "glimmer-object-reference";
import { Opaque } from "glimmer-util";

Expand Down Expand Up @@ -409,8 +409,8 @@ test("triple curlies with empty string initial value", assert => {
test("double curlies with const SafeString", assert => {
let rawString = '<b>bold</b> and spicy';

env.setHelper('const-foobar', (args: EvaluatedArgs) => {
return new ConstReference<Opaque>(makeSafeString(rawString));
env.registerInternalHelper('const-foobar', (args: EvaluatedArgs) => {
return new ValueReference<Opaque>(makeSafeString(rawString));
});

let template = compile('<div>{{const-foobar}}</div>');
Expand All @@ -430,8 +430,8 @@ test("double curlies with const SafeString", assert => {
test("double curlies with const Node", assert => {
let rawString = '<b>bold</b> and spicy';

env.setHelper('const-foobar', (args: EvaluatedArgs) => {
return new ConstReference<Opaque>(document.createTextNode(rawString));
env.registerInternalHelper('const-foobar', (args: EvaluatedArgs) => {
return new ValueReference<Opaque>(document.createTextNode(rawString));
});

let template = compile('<div>{{const-foobar}}</div>');
Expand All @@ -451,8 +451,8 @@ test("double curlies with const Node", assert => {
test("triple curlies with const SafeString", assert => {
let rawString = '<b>bold</b> and spicy';

env.setHelper('const-foobar', (args: EvaluatedArgs) => {
return new ConstReference<Opaque>(makeSafeString(rawString));
env.registerInternalHelper('const-foobar', (args: EvaluatedArgs) => {
return new ValueReference<Opaque>(makeSafeString(rawString));
});

let template = compile('<div>{{{const-foobar}}}</div>');
Expand All @@ -472,8 +472,8 @@ test("triple curlies with const SafeString", assert => {
test("triple curlies with const Node", assert => {
let rawString = '<b>bold</b> and spicy';

env.setHelper('const-foobar', (args: EvaluatedArgs) => {
return new ConstReference<Opaque>(document.createTextNode(rawString));
env.registerInternalHelper('const-foobar', (args: EvaluatedArgs) => {
return new ValueReference<Opaque>(document.createTextNode(rawString));
});

let template = compile('<div>{{{const-foobar}}}</div>');
Expand Down
6 changes: 5 additions & 1 deletion packages/glimmer-test-helpers/lib/environment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,10 @@ class HelperReference implements Reference<Opaque> {

return helper(positional.value(), named.value());
}

get(prop: InternedString): SimplePathReference<Opaque> {
return new SimplePathReference(this, prop);
}
}

export class TestEnvironment extends Environment {
Expand All @@ -426,7 +430,7 @@ export class TestEnvironment extends Environment {
this.helpers[name] = (args: EvaluatedArgs) => new HelperReference(helper, args);
}

setHelper(name: string, helper: GlimmerHelper) {
registerInternalHelper(name: string, helper: GlimmerHelper) {
this.helpers[name] = helper;
}

Expand Down

0 comments on commit 8001311

Please sign in to comment.