[Bug] [3.23.0] Helper recomputation regression

[boris-petrov]

🐞 Describe the Bug

A helper doesn't recompute when its argument changes.

🔬 Minimal Reproduction

This repo reproduces the problem. Running the app will show only "5" and not change to "10" (which it must). Downgrading Ember's version to 3.22 (or uncommenting the commented-out line) fixes the issue.

😕 Actual Behavior

The helper doesn't recompute.

🤔 Expected Behavior

The helper to recompute.

[rwjblue]

The thing that is happening here is that the helper is not recomputing unless the specific argument was accessed (e.g. "entangling" in the auto-tracking sense). This is likely due to changes made in #19160 (which implemented helper managers) to pass in the args proxy instead of eagerly entangling.

I don't have a good intuition about what folks expected originally here (and I don't think Ember.Helper really specified the behaviors here since it wasn't RFC'ed), on one hand it be pretty wasteful if we had re-invoke a helper when nothing it uses has changed, but on the other hand code like originally written in ember-render-helpers (which buschtoens/ember-render-helpers#286 updates) accesses the arguments lazily so it is no longer considered consumed.

[rwjblue]

It is pretty unfortunate, but I think we have to roll back on that change (to use the args proxy) to avoid breaking folks relying on the prior behavior.

[rwjblue]

Thanks for reporting @boris-petrov, and for the minimal demo repo!

[boris-petrov]

@rwjblue - thanks for the comments.

This is definitely a breaking change, however I'm not sure whether the commit that brought it is not "more important" and necessary for further work on the framework. As you see, the workaround here is pretty trivial (and as you say there never was a formal spec for helpers) so you could just call it a won't fix - again - only if there isn't an "easy" fix that won't impede future work.

[rwjblue]

Ya, I'm currently thinking if there is a way to do "both" things. They seem pretty mutually exclusive though.

The one idea that I had was to allow Ember.Helper based helpers to eagerly compute (to fix the breakage), but then to also issue a deprecation if you don't access the tag during compute but you do access it lazily. Doing that won't give us any performance improvements, but it will allow us (in the future) to remove the eager consumption...

[buschtoens]

Maybe expose this config as an optional feature? Or have a new Helper base class, e.g. from the @glimmer scope? It would indeed be quite sad, if users couldn't benefit from this perf imporvement. A gradual migration path would be awesome.

Two further issues I noticed:

• Object.values([1, 2, 3]) ➡️ [1, 2, 3]
Object.values(positionalArgsProxy) ➡️

  Object.keys() was called on the positional arguments array for a helper, which is not supported. This function is a low-level function that should not need to be called for positional argument arrays. You may be attempting to iterate over the array using for...in instead of for...of.

  Is this really intentional?

• When iterating over entries, how do positional and named args proxies behave, when new entries are inserted, e.g. positional.pushObject('foo') or named.set('foo', 'bar')? Is this scenario somehow possible to achieve or would a change like this always invalidate the whole Helper, as it requires a different invocation in the template?

[rwjblue]

Object.keys() was called on the positional arguments array for a helper, which is not supported. This function is a low-level function that should not need to be called for positional argument arrays. You may be attempting to iterate over the array using for...in instead of for...of.

Is this really intentional?

No. Can you open a specific issue for that?

[sandstrom]

As you see, the workaround here is pretty trivial (and as you say there never was a formal spec for helpers) so you could just call it a won't fix

I'm interpreting this as the OP being fine with closing.

By closing some old issues we reduce the list of open issues to a more manageable set. Let me know if you think this is a mistake and that the issue should stay open.