Skip to content
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

Default Helper Manager #756

Merged
merged 8 commits into from
Oct 15, 2021
Merged

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Jul 18, 2021

@NullVoxPopuli NullVoxPopuli mentioned this pull request Jul 18, 2021
text/0756-helper-default-manager.md Outdated Show resolved Hide resolved
text/0756-helper-default-manager.md Outdated Show resolved Hide resolved
text/0756-helper-default-manager.md Outdated Show resolved Hide resolved
text/0756-helper-default-manager.md Outdated Show resolved Hide resolved
text/0756-helper-default-manager.md Outdated Show resolved Hide resolved
text/0756-helper-default-manager.md Outdated Show resolved Hide resolved
text/0756-helper-default-manager.md Show resolved Hide resolved
text/0756-helper-default-manager.md Outdated Show resolved Hide resolved
NullVoxPopuli and others added 4 commits October 5, 2021 13:31
Co-authored-by: Robert Jackson <me@rwjblue.com>
Co-authored-by: Robert Jackson <me@rwjblue.com>
Co-authored-by: Robert Jackson <me@rwjblue.com>
@rwjblue rwjblue self-requested a review October 8, 2021 18:42
@rwjblue rwjblue added T-framework RFCs that impact the ember.js library T-templates labels Oct 8, 2021
@rwjblue
Copy link
Member

rwjblue commented Oct 8, 2021

We discussed this at today's core team meeting and are moving it into final comment period. 🎉 🎉

Copy link
Contributor

@pzuraq pzuraq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks amazing, thanks for putting this together @NullVoxPopuli, really excited about this RFC 😄

@NullVoxPopuli
Copy link
Contributor Author

Thank you for doing the exploratory work 🎉

@chriskrycho
Copy link
Contributor

Sorry for the late comment, but hey, this is why we have FCP! Unfortunately, I’m here to raise a fairly significant issue from the Typed Ember team: I believe it is actually impossible (at least with the current feature set of TypeScript or any anticipated features) to provide types which work correctly with a named-args-as-last-argument and (a) can be robustly type checked or (b) provide useful autocomplete.

I’m working on writing up a longer deep dive explanation which will show why this is so we can dig through it in detail, and we’ll double check that our current view of the situation is correct as part of writing that up. The TL;DR is that even with TS's fancy tuple types, we can't actually keep enough information around for tools like Glint to have the info they need—it's basically the same kind of problem as JS's lack of support for function foo(...positional, named) {}.

In the interim, while I’m working on that, a few notes:

  • When Typed Ember folks and @pzuraq were discussing this last year as we were pushing Glint toward public beta and @pzuraq was working on ember-could-get-used-to-this, our conclusion was that we couldn’t support this format, but would prefer to solve it by just having better support for objects (not needing hash but having some kind of syntax) in templates.

  • It's possible (we haven't dug into it yet) that if this came with a restriction of only allowing named or positional params, but not both, we could make it work.

  • The Typed Ember folks really want this feature to land. It’s a huge improvement for the ecosystem! We just want to make sure that what lands is something that tools like Glint can actually support—especially because we hope long-term to use Glint to supercharge everyone's experience of writing Ember, whether in TS or JS—and it would be a big frustration if whole classes of helpers just don't work with Glint.

Note: while I’m working on getting this written up, I would appreciate it if we skipped any proposals for workarounds/etc. It’ll be much more productive if we can avoid going around about what does/n’t work until we have a clear explanation of the constraints in place!

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Oct 12, 2021

thanks for the explanation and thoughts!
atm my preference is then to propose only positional arguments as only named arguments would be very verbose, and not how folks are used to writing functions anyway.

that said, I don't think dropping support for named arguments today prevents the addition of them in the future, like, if TC39&co do decide to add support for fun(...rest, last) (I think they should! (and for array destructuring too...)), we can then add support later as a purely additive change with no breaking changes. 🎉

@chriskrycho
Copy link
Contributor

chriskrycho commented Oct 12, 2021

All right, having spent a few hours doing ✨ DARK TS MAGIC ✨ I have actually found a way to make this work, so consider the concern withdrawn.1 That said, there may be some costs to this: repeatedly doing the kind of overloaded type resolution (using conditional types, no less) for helpers and modifiers in the context of something like Glint will have overhead for type-checking, slowing it down both in build and in-editor feedback.

I will note that getting this to work is quite finicky, and it wasn't possible when we last evaluated it during the Glint private alpha phase. The reason it's finicky is that there's a mismatch between the Handlebars semantics (which look more like Ruby's or Python's keyword argument semantics) and JavaScript semantics. Since JS doesn't have keyword arguments, the Handlebars/Glimmer distinction between named and positional parameters will remain a bad fit with JS/TS semantics for the foreseeable future.2

While I’m happy to see this specific issue unblocked, I hope we revisit the general design of helpers and modifiers and indeed “invokables” during the Ember 4.x/post-Polaris era, because I think there are potentially much better designs here which have much closer alignment with JS semantics.3

Footnotes

  1. If you'd like to see the dark magic at work in the context of Glint's resolution handling, it's available in this playground.

  2. There are no active proposals for such a feature, and it's difficult to imagine how they would fit with the language. But if a proposal came through, then the issues with TS would presumably be much simplified b/c the semantics would have been aligned!

  3. As suggested in my first comment, I continue to think that a syntax which made just using objects easy—no need for hash—would solve this cleanly, and in a way that would both simplify TS handling and make the behavior strictly more general.

@wycats
Copy link
Member

wycats commented Oct 15, 2021

@chriskrycho I strongly agree with you that we should revisit the design after Polaris.

A quick sketch of one direction: instead of helper manager, we could standardize on the "just a JS function" design in this RFC and require that people who want to do more elaborate mapping create higher-order functions that produce the kind of function Ember expects. As long as you type your higher-order function properly, Glint could target the natural mapping from Handlebars function call to JS function call, and everything should just work.

A note on JS idioms: I personally believe that people should not create signatures that combine:

  • variable numbers of positional parameters
  • named parameters

I think sometehing like (format date fmt="...") is fine, but if you have a variable number of positional arguments, you should turn the optional ones into options. Since this RFC de-emphasizes the ergonomics of named arguments in order to align better with JavaScript, I think those idioms will also make sense for helpers (especially user-authored helpers).

@rwjblue
Copy link
Member

rwjblue commented Oct 15, 2021

Thanks for exploring this @chriskrycho @wycats!

The team discussed this today, and (given that @chriskrycho confirmed that this wasn't a fundamental blocker) are going to go ahead and land this RFC.

Thanks again for all of the conversation here y'all!!

NullVoxPopuli added a commit to ember-polyfills/ember-functions-as-helper-polyfill that referenced this pull request Jan 21, 2022
BREAKING CHANGE: copy real implementation
this changes the behavior of the options arg to enable
optional positional params when options are not used.
See the RFC update for more details.
emberjs/rfcs#756
github-actions bot pushed a commit to ember-polyfills/ember-functions-as-helper-polyfill that referenced this pull request Jan 30, 2022
# [2.0.0](v1.0.15...v2.0.0) (2022-01-30)

### chore

* sync with RFC ([343a698](343a698))

### BREAKING CHANGES

* copy real implementation
this changes the behavior of the options arg to enable
optional positional params when options are not used.
See the RFC update for more details.
emberjs/rfcs#756
@Windvis Windvis mentioned this pull request Apr 13, 2022
@chriskrycho chriskrycho mentioned this pull request Apr 19, 2022
@buschtoens
Copy link
Contributor

buschtoens commented Aug 10, 2022

What do y'all think about binding the this context of the function to the owner it is invoked from?

Please let me know, if I should open this as a separate issue instead or start work on an RFC right away.

/** Creates a new ember-intl `t` helper function that is scoped to a prefix. */
export function scopedT(prefix: string) {
  return function (this: Owner, key: string, options: Record<string, unknown>): string {
    return this.lookup('service:intl').t(`${prefix}.${key}`, options);
  }
}

export const tLabel = scopedT('ui.labels');
export const tError = scopedT('ui.error');
<label>{{tLabel "foo"}}</label>
<strong>{{tError "oh-no"}}</strong>

{{#let (scopedT "on-the-fly.bindings") as |t|}}
  <p>{{t "are-super-useful"}}</p>
{{/let}}

@chriskrycho
Copy link
Contributor

@buschtoens we very much want things to be normally bound when invoked… but probably not to the Owner. Instead, we want normal JS binding rules to apply, so if you invoke a method directly on an object, {{foo.bar "baz"}}, you get the normal rules you would if you do in JS, foo.bar("baz"). If you’re interested in helping make that work, please let us know, because we have a number of ideas about how to do it, but currently it's not at the top of anyone's list for implementation efforts! And, to be frank, we could especially use to get more knowledge of how the relevant bits of Glimmer work distributed into more heads. 😅

@buschtoens
Copy link
Contributor

buschtoens commented Aug 10, 2022

@chriskrycho Thank you for the super-swift reply! ⚡

I am 💯 on-board with what you're saying about retaining normal JS binding rules for method invocations, i.e. {{foo.bar "baz"}}foo.bar("baz").

(And I'm very interested in helping! In general, I would like to familiarize myself more with the Glimmer internals. So far I only ever really ventured in there, when fixing bugs.)

Coming back to the owner as this binding idea: I might have expressed myself unclearly. I did not mean that an invocation like {{foo.bar "baz"}} should be bound to the owner instead of foo, though this is an interesting aspect, that I didn't explicitly consider before.

As a matter of fact "bind" might be the wrong term to use. The function / method itself should not be bound to the owner as fn.bind(owner) would. Instead the owner should be set as the this context, when the function is invoked, e.g. fn.call(owner, ...args).

IMO this can co-exist with {{foo.bar "baz"}} being "bound" to foo, when only simple identifiers ({{bar "baz"}}) receive owner as this. Key paths would always use the second-to-last segment, like JS ({{foo.bar.qux "baz"}}foo.bar.qux('baz')). In the same way, if you "chop off" a method and only pass that reference, then it shouldn't be bound to its host object any more, just like in JS.

{{foo.bar "baz"}} → bound to `foo`

{{#let foo.bar as |bar|}}
  {{bar "baz"}} → bound to owner
{{/let}}

I've implemented a quick PoC for the polyfill. Of course it doesn't yet have any special handling for key paths.
ember-polyfills/ember-functions-as-helper-polyfill#132

Your mention of key paths got me thinking though: Maybe we shouldn't use the owner for this, but the actual template context, i.e. the backing component class instance. The owner could be obtained from there.

I can imagine scenarios, where convenient access to the template context without explicitly requiring the user to pass this as an extra argument is useful. This also means that helper functions would could use getOwner(this) consistently in both scenarios, assuming that if they're invoked via a host object ({{foo.bar "baz"}}), that host object does have an associated owner.

@chriskrycho
Copy link
Contributor

First—super excited to hear you're interested in the other bit; I'll chat with other Framework folks and see if we can get someone engaged to help that get moving forward with you!

Second—on reading your original post again I follow what you’re wanting this for. Unfortunately, in that case, there's a fundamental problem (and it goes for any other general this binding of functions as well): arrow functions, which explicitly cannot have their this set via .call() or otherwise. Also, that could seriously mess up people relying on the ability to use .bind themselves! So I think the idea of auto-binding to the Owner isn't going to work… because JavaScript™.

Also, note that for all template-only components, there is a template context, but not necessarily a backing component class instance: this is expressly null for template-only components.

All that being said, I think there's an interesting design question around making it possible for functions to interact more directly with the DI container, even if I don't think we can do it in a direct way via function binding. One possibility in the space is something shaped sort of like React's getContext() hook—we don't have a design for something like that yet, though. Another possibility is to simply keep using class-based helpers that way!

import Helper from '@ember/component/helper';
import { service } from '@ember/service';
import IntlService from 'wherever/it/comes/from';

interface ScopedTSig {
  Args: {
    Named: Record<string, unknown>;
    Positional: [prefix: string];
  };
}

class ScopedT extends Helper<ScopedTSig> {
  @service declare intl: IntlService;

  compute([prefix]: [string], options: Record<string, unknown>) {
    return this.intl.t(`${prefix}.${key}`, options);
  }
}

<template>
  {{ScopedT 'some-key' makeItAwesome=true}}
</template>

This is definitely a bit longer, but it gets the job done nicely. I’ve also written a tiny class-based helper in the past to just pass along a service:

import Helper from '@ember/component/helper';
import { getOwner } from '@ember/application';

export default class GetService extends Helper {
  compute([name]) {
    return getOwner(this).lookup(`service:${name}`);
  }
}
import GetService from 'my-app/helpers/get-service';
import someHelperWhichNeedsFoo from 'my-app/helpers/some-helper';

<template>
  {{someHelperWhichNeedsFoo foo=(getService 'foo')}}
<template>

Given recent TS work, that could even in principle become well-typed to return a known service with some tweaks (happy to chat about that on Discord).

None of that changes the fact that something shaped like getContext() would be quite nice.

ef4 added a commit that referenced this pull request Feb 3, 2023
Advance RFC #756 to Stage Recommended
chancancode added a commit to tildeio/discourse that referenced this pull request Jun 15, 2023
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
CvX pushed a commit to discourse/discourse that referenced this pull request Jun 15, 2023
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Final Comment Period T-framework RFCs that impact the ember.js library T-templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants