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

Make the helper keyword handling less strict #1150

Merged
merged 3 commits into from
Mar 7, 2022
Merged

Make the helper keyword handling less strict #1150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c3b43d4
Reduce the strictness of the helper keyword handling
Windvis Mar 5, 2022
fcb867d
Merge branch 'main' into fix/helper-keyword-currying
ef4 Mar 7, 2022
69663bc
tests were sensitive to nondeterministic ordering
ef4 Mar 7, 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
17 changes: 7 additions & 10 deletions packages/compat/src/resolver-transform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,16 +333,13 @@ 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':
resolver.resolveDynamicHelper({ type: 'literal', path: param.chars }, moduleName, param.loc);
break;
default:
resolver.resolveDynamicHelper({ type: 'other' }, moduleName, param.loc);
function handleDynamicHelper(param: ASTv1.Expression, resolver: Resolver, moduleName: string): void {
Copy link
Collaborator Author

@Windvis Windvis Mar 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm new to TypeScript but I think Expression is the correct type here since node.params seems to be typed as Expression[]? Expression doesn't include the TextNode type, so that doesn't need to be covered anymore.

(I think that's also the case for the component helper version).

// We only need to handle StringLiterals since Ember already throws an error if unsupported values
// are passed to the helper keyword.
// If a helper reference is passed in we don't need to do anything since it's either the result of a previous
// helper keyword invocation, or a helper reference that was imported somewhere.
if (param.type === 'StringLiteral') {
resolver.resolveDynamicHelper({ type: 'literal', path: param.value }, moduleName, param.loc);
}
}

Expand Down
45 changes: 29 additions & 16 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,7 +519,7 @@ describe('compat-resolver', function () {
},
]);
});
test('string literal passed to `helper` helper in content position', function () {
test('string literal passed to "helper" keyword in content position', function () {
Copy link
Collaborator Author

@Windvis Windvis Mar 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I started renaming "helper" to "keyword" since that's the word they use in the error in ember-source:

Uncaught Error: Assertion Failed: Passing a dynamic string to the (helper) keyword is disallowed. (You specified (helper (concat "hel" "lo")) and (concat "hel" "lo") evaluated into "hello".) 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 "hello"). 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. ('contextual-built-ins/templates/application.hbs' @ L39:C0)

I assumed "keyword" was an implementation detail and not an end-user facing term, but it seems it's used in error messages so I think we can be consistent with that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I agree, it seems clearer to say "keyword"

let findDependencies = configure({
staticHelpers: true,
});
Expand Down Expand Up @@ -584,7 +584,7 @@ describe('compat-resolver', function () {
)
).toEqual([]);
});
test('built-in helpers are ignored when used with the "helper" helper', function () {
test('built-in helpers are ignored when used with the "helper" keyword', function () {
let findDependencies = configure({
staticHelpers: true,
});
Expand Down Expand Up @@ -714,7 +714,7 @@ describe('compat-resolver', function () {
},
]);
});
test('string literal passed to "helper" helper in helper position', function () {
test('string literal passed to "helper" keyword in helper position', function () {
let findDependencies = configure({ staticHelpers: true });
givenFile('helpers/hello-world.js');
expect(
Expand All @@ -733,6 +733,27 @@ describe('compat-resolver', function () {
},
]);
});
test('helper currying using the "helper" keyword', function () {
let findDependencies = configure({ staticHelpers: true });
givenFile('helpers/hello-world.js');
expect(
findDependencies(
'templates/application.hbs',
`
{{#let (helper "hello-world" name="World") as |hello|}}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is based on the code snippet in the RFC.

{{#let (helper hello name="Tomster") as |helloTomster|}}
{{helloTomster name="Zoey"}}
{{/let}}
{{/let}}
`
)
).toEqual([
{
path: '../helpers/hello-world.js',
runtimeName: 'the-app/helpers/hello-world',
},
]);
});
test('string literal passed to "modifier" keyword in helper position', function () {
let findDependencies = configure({ staticModifiers: true });
givenFile('modifiers/add-listener.js');
Expand All @@ -759,7 +780,7 @@ describe('compat-resolver', function () {
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 () {
test('string literal passed to "helper" keyword fails to resolve', function () {
let findDependencies = configure({ staticHelpers: true });
expect(() => {
findDependencies('templates/application.hbs', `{{helper "hello-world"}}`);
Expand All @@ -779,8 +800,9 @@ describe('compat-resolver', function () {
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 () {
test('string literal passed to "helper" keyword fails to resolve when staticHelpers is off', function () {
let findDependencies = configure({ staticHelpers: false });
givenFile('helpers/hello-world.js');
expect(findDependencies('templates/application.hbs', `{{helper "hello-world"}}`)).toEqual([]);
});
test('string literal passed to "modifier" keyword fails to resolve when staticModifiers is off', function () {
Expand Down Expand Up @@ -1854,18 +1876,9 @@ describe('compat-resolver', function () {
);
});

test('rejects arbitrary expression in "helper" helper', function () {
test('ignores any non-string-literal in "helper" keyword', 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`
);
expect(findDependencies('templates/application.hbs', `{{helper this.which}}`)).toEqual([]);
});

test('ignores any non-string-literal in "modifier" keyword', function () {
Expand Down
19 changes: 12 additions & 7 deletions tests/fixtures/engines-host-app/tests/acceptance/basics-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ function arrayOfCSSRules(styleSheets, cssSelector, cssProperty) {
}
}

return values;
return values.sort();
}

// We don't yet support lazy CSS in apps that are using fastboot. This test
Expand All @@ -39,11 +39,14 @@ module('Acceptance | basics', function (hooks) {
let rules = arrayOfCSSRules(document.styleSheets, '.shared-style-target', 'content');

if (ensureCSSisLazy) {
assert.deepEqual(rules, [
'engines-host-app/vendor/styles.css',
'eager-engine/addon/styles/addon.css',
'engines-host-app/app/styles/app.css',
]);
assert.deepEqual(
rules,
[
'engines-host-app/vendor/styles.css',
'eager-engine/addon/styles/addon.css',
'engines-host-app/app/styles/app.css',
].sort()
);
}

await visit('/style-check');
Expand Down Expand Up @@ -91,7 +94,9 @@ module('Acceptance | basics', function (hooks) {
'macro-sample-addon/addon/styles/addon.css',
'lazy-engine/addon/styles/addon.css',
ensureCSSisLazy ? undefined : 'lazy-in-repo-engine/addon/styles/addon.css',
].filter(Boolean)
]
.filter(Boolean)
.sort()
);

await visit('/style-check');
Expand Down