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.

Sign up for GitHub

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

Jump to bottom

Add support for the helper helper #1120

Merged
merged 5 commits into from
Feb 28, 2022
Merged

Add support for the helper helper #1120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
97d10f3
Update the ember-template-compiler to v4.1.0
Windvis Feb 3, 2022
42d8dc4
Reuse the loc of the macroMaybeAttrs hashes for the created attributes
Windvis Feb 4, 2022
6bedb73
Add support for the `helper` helper
Windvis Feb 8, 2022
f9f8f6f
Merge branch 'main' into feature/support-dynamic-helpers
ef4 Feb 28, 2022
fbf17fa
finishing "helpe" helper support
ef4 Feb 28, 2022
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
21 changes: 21 additions & 0 deletions packages/compat/src/resolver-transform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ export function makeResolverTransform(resolver: Resolver) {
handleComponentHelper(node.params[0], resolver, filename, scopeStack);
return;
}
if (node.path.original === 'helper' && node.params.length > 0) {
handleDynamicHelper(node.params[0], resolver, filename);
return;
}
resolver.resolveSubExpression(node.path.original, filename, node.path.loc);
},
MustacheStatement(node: ASTv1.MustacheStatement) {
Expand All @@ -97,6 +101,10 @@ export function makeResolverTransform(resolver: Resolver) {
handleComponentHelper(node.params[0], resolver, filename, scopeStack);
return;
}
if (node.path.original === 'helper' && node.params.length > 0) {
handleDynamicHelper(node.params[0], resolver, filename);
return;
}
let hasArgs = node.params.length > 0 || node.hash.pairs.length > 0;
let resolution = resolver.resolveMustache(node.path.original, hasArgs, filename, node.path.loc);
if (resolution && resolution.type === 'component') {
Expand Down Expand Up @@ -320,3 +328,16 @@ 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':
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silly question, but how is it possible to pass a TextNode to the helper keyword? There is probably a legit use-case I'm missing, but I can't seem to create that scenario in AST-explorer 😄.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's not? I was copying the earlier work on the component helper and that does have handling for TextNode, and I don't recall if that's because we found a case or because some typescript types implied it was possible.

resolver.resolveDynamicHelper({ type: 'literal', path: param.chars }, moduleName, param.loc);
break;
default:
resolver.resolveDynamicHelper({ type: 'other' }, moduleName, param.loc);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ef4 Do we actually want to handle this case? The contextual helpers RFCs mentions that passing a helper reference into the helper should be supported so that extra arguments can be added: https://emberjs.github.io/rfcs/0432-contextual-helpers.html

I left it out in the draft PR since Ember itself verifies that the helper isn't dynamic. I think Embroider only needs to do anything if users pass in a helper name?

I think the code snippet in the RFC would throw an error with the current implementation?

{{#let (helper "join-words" separator=",") as |join|}}
  {{#let (helper join "foo") as |foo|}}
    {{#let (helper foo "bar") as |foo-bar|}}
      {{foo-bar "baz"}}
    {{/let}}
  {{/let}}
{{/let}}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this also generates an error, it's just done from the resolver instead of directly here because the resolver keeps track of all the errors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I think it shouldn't throw an error for that code snippet? Passing in a helper reference should work, according to the RFC, but with the currently merged implementation it will throw an error I think?

Unless I misunderstood the code, I think at the moment, errors will be thrown under Embroider while it works as expected in a classic build.

}
}
40 changes: 39 additions & 1 deletion packages/compat/src/resolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,14 @@ const builtInHelpers = [
'hasBlock',
'hasBlockParams',
'hash',
'helper',
'if',
'input',
'let',
'link-to',
'loc',
'log',
// 'modifier',
'mount',
'mut',
'on',
Expand All @@ -108,7 +110,6 @@ const builtInHelpers = [
];

const builtInComponents = ['input', 'link-to', 'textarea'];

const builtInModifiers = ['action', 'on'];

// this is a subset of the full Options. We care about serializability, and we
Expand Down Expand Up @@ -821,6 +822,43 @@ export default class CompatResolver implements Resolver {
from
);
}

resolveDynamicHelper(helper: ComponentLocator, from: string, loc: Loc): Resolution | null {
if (!this.staticHelpersEnabled) {
return null;
}

if (helper.type === 'literal') {
let helperName = helper.path;
if (builtInHelpers.includes(helperName)) {
return null;
}

let found = this.tryHelper(helperName, from);
if (found) {
return this.add(found, from);
}
return this.add(
{
type: 'error',
message: `Missing helper`,
detail: helperName,
loc,
},
from
);
} else {
return this.add(
{
type: 'error',
message: 'Unsafe dynamic helper',
detail: `cannot statically analyze this expression`,
loc,
},
from
);
}
}
}

function humanReadableFile(root: string, file: string) {
Expand Down
70 changes: 70 additions & 0 deletions packages/compat/tests/resolver.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,18 @@ describe('compat-resolver', function () {
},
]);
});
test('string literal passed to `helper` helper in content position', function () {
let findDependencies = configure({
staticHelpers: true,
});
givenFile('helpers/hello-world.js');
expect(findDependencies('templates/application.hbs', `{{helper "hello-world"}}`)).toEqual([
{
path: '../helpers/hello-world.js',
runtimeName: 'the-app/helpers/hello-world',
},
]);
});
test('built-in components are ignored when used with the component helper', function () {
let findDependencies = configure({
staticComponents: true,
Expand All @@ -534,6 +546,21 @@ describe('compat-resolver', function () {
)
).toEqual([]);
});
test('built-in helpers are ignored when used with the "helper" helper', function () {
let findDependencies = configure({
staticHelpers: true,
});
expect(
findDependencies(
'templates/application.hbs',
`
{{helper "fn"}}
{{helper "array"}}
{{helper "concat"}}
`
)
).toEqual([]);
});
test('component helper with direct addon package reference', function () {
let findDependencies = configure({
staticComponents: true,
Expand Down Expand Up @@ -635,18 +662,47 @@ describe('compat-resolver', function () {
},
]);
});
test('string literal passed to "helper" helper in helper position', function () {
let findDependencies = configure({ staticHelpers: true });
givenFile('helpers/hello-world.js');
expect(
findDependencies(
'templates/application.hbs',
`
{{#let (helper "hello-world") as |helloWorld|}}
{{helloWorld}}
{{/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 () {
let findDependencies = configure({ staticHelpers: true });
expect(() => {
findDependencies('templates/application.hbs', `{{helper "hello-world"}}`);
}).toThrow(new RegExp(`Missing helper: hello-world in templates/application.hbs`));
});
test('string literal passed to component helper fails to resolve when staticComponents is off', function () {
let findDependencies = configure({ staticComponents: false });
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 () {
let findDependencies = configure({ staticHelpers: false });
expect(findDependencies('templates/application.hbs', `{{helper "hello-world"}}`)).toEqual([]);
});
test('dynamic component helper error in content position', function () {
let findDependencies = configure({ staticComponents: true });
givenFile('components/hello-world.js');
Expand Down Expand Up @@ -1708,6 +1764,20 @@ describe('compat-resolver', function () {
);
});

test('rejects arbitrary expression in "helper" helper', 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`
);
});

test('trusts inline ensure-safe-component helper', function () {
let findDependencies = configure({ staticComponents: true });
expect(findDependencies('templates/application.hbs', `{{component (ensure-safe-component this.which) }}`)).toEqual(
Expand Down