[FEAT] Implements invokeHelper #19171
Conversation
| return getValue(DEBUG ? ARGS_CACHES!.get(proxy)! : proxy.argsCache!)!; | ||
| } | ||
|
|
||
| class SimpleArgsProxy { |
There was a problem hiding this comment.
Does something like this already exist for modifiers?
There was a problem hiding this comment.
It's a more complicated version which proxies each individual argument. Using that would require users to pass in each arg as a function, which would be a bit prohibitive and is not what was spec'd. So, this one is a simpler proxy that basically calls the users compute function and then returns the proper value whenever positional or named is accessed.
| } | ||
|
|
||
| let instance = managers.get(factory); | ||
|
|
||
| if (instance === undefined) { | ||
| instance = factory(owner); | ||
| instance = factory(owner!); |
There was a problem hiding this comment.
How do you know that owner exists here?
There was a problem hiding this comment.
It may actually be undefined, in the case of helper managers, but for modifier/component managers it will always exist at this point (since you are not currently allowed to pass an undefined owner to getModifierManager or getComponentManager). The types here are a bit messy, but I think it should be fine.
There was a problem hiding this comment.
Looking really good! I left a few inline comments/questions, but I think we are also missing some test cases:
- Usage of
invokeHelperwith custom helper managers (will need to be guarded by both feature flags, but should still be tested) - Ensuring that associating destructors to the the helper context works properly (for both
Ember.Helper/Ember.Helper.helperhelpers and custom helpers) - Usage of non-class based helpers
- Confirming the caching of the
computeArgs, specifically how many times the third argument is expected to be invoked in various contexts - Confirming that a helper that accesses none of the args (positional or named), and nothing else is "constant"
- Confirming that a helper that accesses only args that themselves are constant (e.g. the
computeArgsfunction is something that uses no tags), is constant
packages/@ember/-internals/glimmer/tests/integration/helpers/invoke-helper-test.js
Outdated
Show resolved
Hide resolved
packages/@ember/-internals/glimmer/tests/integration/helpers/invoke-helper-test.js
Outdated
Show resolved
Hide resolved
packages/@ember/-internals/glimmer/tests/integration/helpers/invoke-helper-test.js
Outdated
Show resolved
Hide resolved
pzuraq
force-pushed
the
invoke-helper
branch
4 times, most recently
from
6f3eb2a
to
e4c0450
Compare
| assert.equal(count, 2, 'helper called a second time'); | ||
| } | ||
|
|
||
| '@test helper that with constant args is costant'(assert) { |
| } | ||
| } | ||
| ); | ||
|
|
There was a problem hiding this comment.
I think this needs to be:
| if ( | |
| EMBER_GLIMMER_HELPER_MANAGER && | |
| EMBER_GLIMMER_INVOKE_HELPER | |
| ) { |
Specifically, if we enable one before the other (which almost always does happen) these tests may fail. As far as I can tell both features are required for these tests...
There was a problem hiding this comment.
probably hard to see but it was nested in the other if, I can see how that's confusing though. Updated to check both 👍
There was a problem hiding this comment.
Ah, gotcha. The indentation was hard to spot on mobile for sure.
|
🎂👌💫 |
Reopening #19165, thanks for getting it started @NullVoxPopuli!
Implements
invokeHelperas described in RFC 626. Everything is behind a feature flag, ready to flip!