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

Add support for the helper helper #1120

Merged
merged 5 commits into from
Feb 28, 2022

Conversation

Windvis
Copy link
Collaborator

@Windvis Windvis commented Feb 8, 2022

No description provided.

This allows us to add tests for the new `helper` and `modifier` helpers.
The newer template compiler sorts attributes based on their `loc` location when printing. Since previously no `loc` info was added to the newly created attributes, they were being moved to the front of the element. By reusing their original `loc` data we can ensure that the order from the source file is maintained.
// 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.

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

@ef4 ef4 marked this pull request as ready for review February 28, 2022 21:59
@ef4
Copy link
Contributor

ef4 commented Feb 28, 2022

Thanks @Windvis, I was able to push this over the line.

@ef4 ef4 merged commit 2181bf8 into embroider-build:main Feb 28, 2022
@Windvis Windvis deleted the feature/support-dynamic-helpers branch March 1, 2022 08:39
@Windvis
Copy link
Collaborator Author

Windvis commented Mar 1, 2022

@ef4 Sorry about that. I haven't had the time nor energy to do much open source work the past weeks. 😞

Thanks for finishing it up!

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.

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.

@rwjblue rwjblue added the enhancement New feature or request label Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants