Add support for the helper helper

[Windvis]

No description provided.

[lifeart]

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

[Windvis]

@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.

[ef4]

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() {...});
}

[lifeart]

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

[lifeart]

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

[ef4]

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.

[ef4]

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

[ef4]

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

[Windvis]

@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!

[Windvis]

@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}}

[ef4]

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.

[Windvis]

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.

[Windvis]

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 😄.

[ef4]

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.