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 3 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
56 changes: 56 additions & 0 deletions packages/compat/src/resolver-transform.ts
View file
Open in desktop
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 @@ -350,3 +358,51 @@ function handleComponentHelper(

// resolver.unresolvableComponentArgument(componentName, argumentName, moduleName, param.loc);
}

function handleDynamicHelper(
param: ASTv1.Node,
resolver: Resolver,
moduleName: string
// scopeStack: ScopeStack,
): void {
// let locator: ComponentLocator;
// switch (param.type) {
// case 'StringLiteral':
// locator = { type: 'literal', path: param.value };
// break;
// case 'PathExpression':
// locator = { type: 'path', path: param.original };
// break;
// case 'MustacheStatement':
// if (param.hash.pairs.length === 0 && param.params.length === 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

it will be great to resolve super simple dynamic cases, like

{{helper (if this.a "foo" "bar")}}
{{helper (if this.localHelper this.localHelper "fallback-helper")}}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lifeart I think it was a deliberate choice not to support that for these helpers. #1018 (comment)

Maybe I just misunderstood, but the RFC doesn't mention that either I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the dynamic case can be dynamic outside of the (helper ) call:

{{if this.localHelper this.localHelper (helper "fallback-helper") }}

But also, since helpers are first-class values that can be passed around IMO it's nicer to implement your fallback-helper in the corresponding JS file anyway, so that it's not polluting the global namespace:

{{if this.localHelper this.localHelper this.fallbackHelper }}
import { helper } from '@ember/component/helper';
import Component from '@glimmer/component';
export default class extends Component {
  fallbackHelper = helper(function() {...});
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ef4 looks like it could be proposal for ember-template-lint rule, like "embroider-ready" to have list of this checks and recomendations

Copy link
Collaborator

Choose a reason for hiding this comment

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

ember-template-lint/ember-template-lint#2375

Copy link
Contributor

Choose a reason for hiding this comment

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

I confirmed that ember already hard-enforces the main thing we care about here:

Assertion Failed: Passing a dynamic string to the (helper) keyword is disallowed. (You specified (helper s) and s evaluated into "my-helper".) This ensures we can statically analyze the template and determine which helpers are used. If the helper name is always the same, use a string literal instead, i.e. (helper "my-helper"). Otherwise, import the helpers into JavaScript and pass them to the helper keyword. See https://github.com/emberjs/rfcs/blob/master/text/0496-handlebars-strict-mode.md#4-no-dynamic-resolution for details. ('sample2/templates/application.hbs' @ L3:C9)

The last bit of that message is actually out of date for Ember 3.25+ because you don't need to even pass them to the helper keyword, you can just use them as-is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fired off a PR to update those out-of-date instructions in Ember emberjs/ember.js#19950

// handleComponentHelper(param.path, resolver, moduleName, scopeStack, impliedBecause);
// return;
// } else if (param.path.type === 'PathExpression' && param.path.original === 'component') {
// // safe because we will handle this inner `{{component ...}}` mustache on its own
// return;
// } else {
// locator = { type: 'other' };
// }
// break;
// case 'TextNode':
// locator = { type: 'literal', path: param.chars };
// break;
// case 'SubExpression':
// if (param.path.type === 'PathExpression' && param.path.original === 'component') {
// // safe because we will handle this inner `(component ...)` subexpression on its own
// return;
// }
// if (param.path.type === 'PathExpression' && param.path.original === 'ensure-safe-component') {
// // safe because we trust ensure-safe-component
// return;
// }
// locator = { type: 'other' };
// break;
// default:
// locator = { type: 'other' };
// }

if (param.type === 'StringLiteral') {
resolver.resolveDynamicHelper(param.value, moduleName, param.loc);
}
}
27 changes: 26 additions & 1 deletion packages/compat/src/resolver.ts
View file
Open in desktop
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,30 @@ export default class CompatResolver implements Resolver {
from
);
}

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

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
);
}
}

function humanReadableFile(root: string, file: string) {
Expand Down
58 changes: 57 additions & 1 deletion packages/compat/tests/resolver.test.ts
View file
Open in desktop
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 @@ -874,7 +930,7 @@ describe('compat-resolver', function () {
findDependencies(
'templates/application.hbs',
`
{{outlet "foo"}}
{{outlet}}
{{yield bar}}
{{#with (hash submit=(action doit)) as |thing| }}
{{/with}}
Expand Down
4 changes: 2 additions & 2 deletions packages/macros/src/glimmer/macro-maybe-attrs.ts
View file
Open in desktop
Expand Up @@ -20,11 +20,11 @@ export function maybeAttrs(elementNode: any, node: any, builders: any) {

if (result.value) {
for (let bareAttr of bareAttrs) {
elementNode.attributes.push(builders.attr(bareAttr.original, builders.text('')));
elementNode.attributes.push(builders.attr(bareAttr.original, builders.text(''), bareAttr.loc));
}

for (let attr of node.hash.pairs) {
elementNode.attributes.push(builders.attr(attr.key, builders.mustache(attr.value)));
elementNode.attributes.push(builders.attr(attr.key, builders.mustache(attr.value), attr.loc));
}
}
}
2 changes: 1 addition & 1 deletion test-packages/support/vendor/README.md
View file
Open in desktop
@@ -1,3 +1,3 @@
This is vendored from ember 3.25.0.
This is vendored from ember 4.1.0.

I did it this way because if I try to depend directly on ember-source, I end up with a version of fs-tree-diff that has bad types in it that messes up my build.