-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
in-implementation updates to the default helper manager RFC, 756 #788
in-implementation updates to the default helper manager RFC, 756 #788
Conversation
Notes from our meeting:
|
text/0756-helper-default-manager.md
Outdated
@@ -156,6 +176,68 @@ setHelperManager(() => DEFAULT_HELPER_MANAGER, Function.prototype); | |||
- to register this helper manager, it should occur during app boot so developers do not need to import anything to | |||
trigger the `setHelperManager` call | |||
|
|||
**Update 2022-01-13, during implementation** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this odd to be in it's own section here, why isn't this updating the detailed design just above (or at least also updating the detailed design)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it made sense to me, and i wanted to progression of discovery to be known. I haven't been aware of any 'proper' way to do these kinds of updates 🙃
I no longer have the energy to put in to this, so if someone wants to take it over, that'd be much appreciated |
I think this is currently hung up on the text is still not very clear to everyone who read it/didn't capture everything we want to convey with the change succinctly. I can eventually wrap this up (or someone else could) but it's not a huge priority for me personally, because the feature was landed anyway. It's important to capture this in the RFC as the source of truth but it's not a blocker, though it's important to do it while we still have the context. |
@chancancode do your thoughts remain the same here? |
8a45fcc
to
3ad683b
Compare
913be7b
to
17ee690
Compare
Co-authored-by: Godfrey Chan <[email protected]>
17ee690
to
d9adc36
Compare
This feature landed in Ember 4.5+, but this polyfill would allow it to work on 3.25+ References RFC: emberjs/rfcs#756 Update: emberjs/rfcs#788 Guides: ember-learn/guides-source#1924
* Enable "plain function as helpers" polyfill This feature landed in Ember 4.5+, but this polyfill would allow it to work on 3.25+ References RFC: emberjs/rfcs#756 Update: emberjs/rfcs#788 Guides: ember-learn/guides-source#1924 * Convert truth-helpers to use plain functions Mainly to test that the polyfill is working, but it's a good refactor anyway.
During implementation, a couple things came up:
we don't do a great job of documenting when autotracking happens outside of getters -- therefore, added an example of autotracking in a helper to the examples section -- as composing helpers in classes that have access to tracked data can be a powerful encapsulation tool for library and app authors alike
we need a minor tweak to how arguments are passed to the helper function to enable default arguments for helpers relying on positional args
Much thanks to @chancancode for binging these things up.
Also, Idk how this is supposed to work with staged RFCs, 🤷 I guessed (feedback welcome, as always)
Rendered