[Glimmer] Consistency in how helpers are executed inside and outside subexpressions

[jmurphyau]

The SubexpStream is not passing stream values when executing env.hooks.invokeHelper - leading to inconsistent behaviour

[rwjblue]

This test name is confusing to me. It talks about using unbound helpers, but the test is confirming that the output updates when the value it is retrieving changes (which to me seems like a bound helper).

[jmurphyau]

How about Unbound helpers should have access to streams when being called directly and when being called in a subexpression - the title also becomes more related to the test module

[rwjblue]

Much clearer! Sorry for being pedantic on the naming. I have recently been going through some older tests where their intent is extremely hard to understand from the test title, so I think I am a tad over-sensitive here. 🍧

Thank you!

[rwjblue]

I think that @wycats or @mmun will need to chime in here, but I was under the impression that helpers should always be invoked with resolved values (aka not streams).

[mmun]

This is by design. Helpers are pure function that never see streams.

[jmurphyau]

The last else if block in the invokeHelper hook provides the helper with streams (as if it were an unbound helper)

ember.js/packages/ember-htmlbars/lib/hooks/invoke-helper.js

Line 21 in 8962e36

return { value: helperFunc.call({}, _params, _hash, templates, env, scope) };

.

This PR is more around consistency in keeping the invokeHelper responsible for reading stream values - rather than doing it in the SubexprStream and then again 99% of the time in the invokeHelper stream.. Most helpers get covered with the first if block in the link pasted above - in which case they will be provided resolved values - not streams.

[mmun]

Helpers should never have access to streams. Your test has a helper which is accessing streams. This is not a valid test. What are you trying to do?

[jmurphyau]

I'm trying to get my addon ember-get-helper to work.. I've managed to get it to work everywhere Ok except when used in a subexpression and I narrowed it down to this code.. Because the addon has two arguments and produces a third value that needs to refresh the output I need to use all the private API stuff to output a stream.

I've found with Glimmer how I could get this to work is to trigger an invalidation on one of the two arguments when the return value changes which gets the code executing again and picks up the new value.

I understand that I'm using very private stuff and I'm quietly preparing myself for one day not being able to do what I'm doing but I've always hoped there was some type of roadmap/thought in regards to handling unbound helpers. If you can suggest another route to go to achieve what I'm doing I'm open to suggestions. I tried for hours to get this to work as a keyword but couldn't completely work out it.

Since this change would just bring subexpression in line with the same behaviour as not using subpexpressions I figured I can improve consistency while still fumbling supporting for my addon in - which is used heavily in my app.

[mmun]

@jmurphyau You're right that this ought to be a keyword and, as you noticed, the keyword API is fairly rough which is why it's still private. I will help you migrate your addon to use keywords soon.

[jmurphyau]

@mmun that would be great. Thank you!

[mixonic]

@mmun promised a pairing date, but his ideas on this PR were correct. Helpers will not have access to streams.

@jmurphyau we absolutely want to create a viable public api for ember-get-helper. However I'm going to close this ticket as it doesn't represent the way forward.

[jmurphyau]

@mixonic I'm wondering if maybe another change might be more appropriate then? It seems like the inconsistency in the behaviour might trip someone up in the future (I spend a few hours tracking it down)..

If helpers should never receive streams and this change shouldn't be implemented, should it be considered that no helper ever gets to receive a stream object?

Currently _registerHelper allows - stops typing - hang on.. right as I was going to explain that I remembered that the way I tricked Ember into passing me streams was quite a bit extreme actually and it might not be worth trying to safe guard it.. Basically you need to know to create a helper as an object rather than a function and create it with a helperFunction property that is the helper function - that's the way you can receive streams in your helper function (link)

[jmurphyau]

thanks for the progress with this. Really interested in getting ember-get-helper working with Glimmer.

@mmun any help you can provide whenever you can provide it would be a great help.. I might just be too eager and once more public information is out about keywords and hooks I'll work it out (:

[mmun]

@jmurphyau My plan at this point is to pull it into core. I'll try and do it soon but you're welcome to submit a PR yourself and we can review it.

[rwjblue]

Basically you need to know to create a helper as an object rather than a function and create it with a helperFunction property that is the helper function - that's the way you can receive streams in your helper function

Yeah, we have to keep that path around for Handlebars style helpers (there are many hoops we had to jump through to make them work). I will work on making it a bit harder to do externally though.

[jmurphyau]

@mmun I've managed to work my way through implementing the helper as a keyword. I'll finish it off over the next few hrs or next day or so and open a PR.

I worked out what I was missing when I tried let time - I needed to subscribe the morph to the newly created stream - that triggers the update to the HTML that I was previously missing..

[mmun]

@jmurphyau Ok, I have it somewhat working as well. I'm in the process of refactoring Stream internals to make the implementation cleaner. Feel free to push a branch if you want me to take a look.

[jmurphyau]

@mmun I've pushed it to my fork.

I'll create a PR now for it - more appropriate to discuss this further there than in a semi unrelated closed issue.

[jmurphyau]

See PR #11196 for discussion and potential implementation of the {{get}} keyword.