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

Add new basic helpers to Ember #388

Closed

Conversation

cibernox
Copy link
Contributor

@cibernox cibernox commented Oct 13, 2018

Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

yes please!


I propose to move to core at least:

- `eq`
Copy link
Member

Choose a reason for hiding this comment

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

will that be == or === semantics?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, IMHO, it should be ===

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to maintain backwards compatibility it has to be ===. And even if compatibility wasn't a concern I'd still prefer triple equal

- `add`, `subtract`, and other arithmetic operators.
- `{{#await promise as |value|}}` to render a block conditionally if a promise resolves or fails.

If there is interest in them, I advocate creating standalone RFCS for them.
Copy link
Member

Choose a reason for hiding this comment

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

I agree, this RFC should be scoped to add the mentioned logic helpers, the others would need dedicated discussions.

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

👍- lets do it

text/0000-basic-template-helpers.md Outdated Show resolved Hide resolved
- RFC PR: (leave this empty)
- Ember Issue: (leave this empty)

# (RFC title goes here)
Copy link
Member

Choose a reason for hiding this comment

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

Missing the title

text/0000-basic-template-helpers.md Outdated Show resolved Hide resolved

- `array` (there is an RFC for it: https://github.com/emberjs/rfcs/pull/318)
- `add`, `subtract`, and other arithmetic operators.
- `{{#await promise as |value|}}` to render a block conditionally if a promise resolves or fails.
Copy link
Member

Choose a reason for hiding this comment

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

Definitely useful IMHO, but should be a separate RFC.

Copy link
Member

Choose a reason for hiding this comment

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

@webark
Copy link

webark commented Oct 13, 2018

#152

just including the existing open RFC on this topic for cross reference, especially with regards to the “short circuiting”, abd why this needs to be done in core space... 😞

@cibernox
Copy link
Contributor Author

cibernox commented Oct 14, 2018

@webark I've expanded the information about short circuiting and how it can help making apps do less work.
I've also expanded the information about how an optimizer compiler (that right now is not that smart, but maybe some day) could even completely remove some branches in compile time from the program altogether.

It would be nice if all helpers could short circuit by default, but that's just one of the four reasons to include those helpers into templating engine. I think it's still valuable even disregarding possible optimizations. Allowing all helpers to pull arguments as they use them would be a separate RFC.

@webark
Copy link

webark commented Oct 14, 2018

@cibernox thanks. and ya, i don't want to get into a “purity” debate. I (having never used ember truth helpers in many apps) always found that if i wanted to reach for one of these, more often then not it was due to a data structure that needed improvement, or that the chunk of template where i was wanting these warranted a new component itself. I will miss this “convention check”, but i feel the community has clearly spoken in favor of this, and there are clear benefits to including it in core. so i’m 👍 even if it’s with a 😞

@cibernox cibernox changed the title Move some helpers from ember-truth-helpers into Ember Add new basic helpers to Ember Oct 15, 2018
@Gaurav0
Copy link
Contributor

Gaurav0 commented Oct 15, 2018

Why not just make ember-truth-helpers part of the default blueprint?

@cibernox
Copy link
Contributor Author

@Gaurav0 in the Alternatives section that possibility is discussed and I expose some arguments why I think this should be in the core.

@jonnii
Copy link

jonnii commented Oct 15, 2018

I also find myself by default installing ember-composable-helpers (https://github.com/DockYard/ember-composable-helpers) whenever starting a new ember applications. If there's an appetite for adding new helpers I'd also advocate for:

  • toggle
  • optional
  • contains
  • inc
  • dec

@locks locks added Needs Champion T-framework RFCs that impact the ember.js library labels Oct 15, 2018
@derekwsgray
Copy link

@cibernox thanks. and ya, i don't want to get into a “purity” debate. I (having never used ember truth helpers in many apps) always found that if i wanted to reach for one of these, more often then not it was due to a data structure that needed improvement, or that the chunk of template where i was wanting these warranted a new component itself. I will miss this “convention check”

Totally agree. Have never used them, and found whenever someone on my team tried to add them it was inevitably !DRY and/or like you point out above. If they're going in, I'd much prefer default blueprint over core, so I could remove them. I do agree there are some decent use-cases I've been shown, but in general they require too much discipline to keep things from getting hairy. Or if there was an addition to the ember linter plugin...

@rwjblue
Copy link
Member

rwjblue commented Oct 16, 2018

Or if there was an addition to the ember linter plugin...

FWIW, we do already have a similar concept in the simple-unless rule. The goal is to essentially limit the totally amount of complexity in logical expressions with {{unless but as you might imagine I believe that these logical expressions could use the same underlying logic to determine “complexity”...

@cibernox
Copy link
Contributor Author

@derekwsgray There is one particular use case that is very annoying to do without some basic logic helpers:

{{#each jsonPayload.users as |user|}}
    {{#if (eq user.type "admin")}} 
      Some admin stuff
    {{else}}
      Non admin stuff.
    {{/if}
{{/each}}

You want to do some very simple check on every iteration. You can do it creating a component for the loop or wrapping your POJOs (to create a isAdmin computed property for instance). You can find ways around without adding any logic to your templates, but in those scenarios a little bit of logic in the template is perfectly acceptable. I think that 100% logicless templates can go against convenience in this case.

@webark
Copy link

webark commented Oct 16, 2018

@cibernox and this is where this convo in something like discord would probably be better.. or if github did threads, but i think this can easily be an example of the later. You could argue something like isAdmin isn’t a “semantic” name. A helper or computed for things like “canEdit” “canDelete” etc cause you could have multiple levels of admins that can perform a range of different actions. You could also argue that what ever you are including there, if it’s somethig like an edit button for like a client view of a cms, having that as a reusable tagless component that you could just drop in different places would have use. I’ve personally seen this kinda of check be littered in react, rails and php markup and then you change who can edit a page for instance when you add an admin type, and now you have to go around and update a bunch of templates, and they maybe you passed in the type as it’s own variable or you passed in user as admin or something.. it’s easy to miss those and manage those.

But again.. this is something that is very subjective. In regards to the helpers, especially with the “short circuiting” of logic values, and the community desire, needs to be in core. You can still purposely not use them, but that can be a choice for an individual and/or a team. The community has clearly spoken in these RFC’s and the download numbers.

@Gaurav0
Copy link
Contributor

Gaurav0 commented Oct 16, 2018

One of the problems with adding stuff like this to core is what happens when a user defines a helper with the same name? Which one wins? How do we ensure that users update ember-truth-helpers to a version without "core" ones?

@cibernox
Copy link
Contributor Author

cibernox commented Oct 16, 2018

@Gaurav0 From the RFC.

When this feature is implemented, we would update ember-truth-helpers to automatically remove the promoted helpers from the code when the Ember is above a certain version number.

This update can happen in a patch as it's not a breaking change and anyone that has a floating version number will eventually get it. We might want to be smart and detect that if they are only using of the ported helpers (no is-equal or is-empty...) and ask them to remove the addon.

About what wins, I think we should just do whatever we do today with built-in helpers like action.

@Gaurav0
Copy link
Contributor

Gaurav0 commented Oct 16, 2018

When this feature is implemented, we would update ember-truth-helpers to automatically remove the promoted helpers from the code when the Ember is above a certain version number.

Doesn't this rely on the user updating to the new patch version of ember-truth-helpers? (Not guaranteed, I feel like I'm the only one at my company who ever tries to yarn upgrade.)

@cibernox
Copy link
Contributor Author

cibernox commented Oct 16, 2018

@Gaurav0 correct, if they don't update and the user-defined helpers clover the default ones (which I don't know if it's the case) they wouldn't get the improvement.
Would you prefer Ember throwing an error if a user defines a helper whose name collides with a default one? That error could ask people to update (or remove) their version of ember-truth-helpers.
That approach sounds like it would be a bit more annoying but would bring more people on the latest best practice.
I also believe that ember-cli-update(which is becoming more popular by the day is it is the recommended way to update versions now ) can run additional tasks, and one of them could bump ember-truth-helpers to the latest version if your app happens to use it.

@rwjblue any insight on what is the best upgrade path?

@Gaurav0
Copy link
Contributor

Gaurav0 commented Oct 16, 2018

Would you prefer Ember throwing an error if a user defines a helper whose name collides with a default one?

I think that would be a great idea. Of course, it would have to be a deprecation warning in the current 3.x cycle, and an error in 4.x actually. Too late for this situation. Probably needs its own RFC.

@derekwsgray
Copy link

Agree with the first part of @webark - the 'admin' example given is not great. The templates become harder to maintain against code changes as you say.

Download numbers don't really sway my opinion (if they did, I'd be using a different framework, har har!). I respect the thought-leaders and proponents of best-practices. If the core ember team thinks this is a good idea, I respect that.
I do concede the argument for "attracting more people in from other frameworks" is persuasive (though doesn't speak to whether this should be blueprint vs. core.)

@GavinJoyce
Copy link
Member

GavinJoyce commented Feb 12, 2019

2. We are copying ember-truth-helpers

I don't have any reason to think that the helpers in ember-truth-helpers have an undesirable implementation. I believe that we should be careful though to ensure that we are designing the behaviour of these new built in helpers from first principles and not just copying existing behaviour because it already exists.

@Panman82
Copy link

What is the state of the discussion of truthiness for these new helpers? I believe some helpers like {{or}} will become even more common to give provide default values once @arguments are more used in glimmer components:

<button type={{or @type "button"}}>...</button>

Related discussion here: #479

IMO, we should also add a {{default}} helper (or such) to return the second parameter when the first is undefined. Because, {{or}} would return the second parameter when the first is falsy, which might be needed.

@miguelcobain
Copy link
Contributor

miguelcobain commented Apr 19, 2019

IMO, we should also add a {{default}} helper (or such) to return the second parameter when the first is undefined. Because, {{or}} would return the second parameter when the first is falsy, which might be needed.

In database systems there is an operation called coalesce. It takes N arguments and returns the first that is non-null.
Example in Postgres: https://www.postgresql.org/docs/9.2/functions-conditional.html#FUNCTIONS-COALESCE-NVL-IFNULL

I could see myself using a coalesce helper that returns the first non-undefined argument. The helper name is debatable.
Example:

{{coalesce @arg1 @arg2 this.value "default value"}}

@cibernox
Copy link
Contributor Author

I think that a default helper could be nice in cases where false is a valid value.

Regardless of that, we must resuscitate the discussion about truthyness. As far as I can remember that was the only controversy

/cc @chancancode @wycats

@pzuraq
Copy link
Contributor

pzuraq commented Apr 19, 2019

I think a {{nullish}} coalescing operator may be more inline with where JS is heading (though that is only a stage 1 proposal still). I also think it may be a better idea to try to get the primitive/basic helpers finalized and added first, before we look into adding a defaulting helper.

- `gt` and `gte`
- `lt` and `lte`

Those helpers are very low lever and generally useful in both Ember and Glimmer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Those helpers are very low lever and generally useful in both Ember and Glimmer.
Those helpers are very low level and generally useful in both Ember and Glimmer.

@mehulkar
Copy link
Contributor

mehulkar commented Jun 4, 2019

Apart from the truthiness discussion, adding more helpers blurs the line between Handlebars and Ember's flavor of Handlebars. I imagine as more added, it will be confusing to user just where to go for documentation on any part of the template they are looking at.

An explicitly allowed list of template helpers (or template imports or strict mode #496) would help mitigate this problem, as it would be easier to see where these magic bits come from.

Or we could decide this is a moot point, considering that package.json contains "ember-cli-htmlbars" and not "handlebars".

@Panman82
Copy link

Panman82 commented Oct 15, 2019

FWIW, I'd like to see this move forward so that I can support optional dynamic tag name (once implemented) in a template only Glimmer Component from an addon without forcing another dep by using ember-truth-helpers.

{{#let (element (or @tagName "div")) as |DynamicElement|}}
    <DynamicElement />
{{/let}}

I realize I could just make that into an {{#if}} block but that would force the need to duplicate everything within the {{#let}} block.

{{#if @tagName}}
    {{#let (element @tagName) as |DynamicElement|}}
        <DynamicElement ...attributes class="foobar"></DynamicElement>
        {{!-- everything here... ---}}
    {{/let}}
{{else}}
    <div ...attributes class="foobar"></div>
    {{!-- would also need to be here --}}
{{/if}}

@chriskrycho
Copy link
Contributor

@Panman8201 I believe you could just use an inline if to work around as well?

{{#let (element (if @tagName @tagName "div")) as |DynamicElement|}}
    <DynamicElement />
{{/let}}

(I'd also like this to move forward, just noting it's not blocking!)

Glimmer VM and passed to the helpers. This might include computationally expensive computed properties,
or hitting code paths that trigger network requests.

Implementing helpers like `and` or `or` at a lower lever would allow those helpers to be evaluated in
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Implementing helpers like `and` or `or` at a lower lever would allow those helpers to be evaluated in
Implementing helpers like `and` or `or` at a lower level would allow those helpers to be evaluated in

@mcfiredrill
Copy link

Is something blocking this? It's unclear to me what the status is.

@mixonic
Copy link
Member

mixonic commented Oct 17, 2019

I raised this in a few recent FW core meetings and it fell to @rwjblue and I to leave some of the feedback here. I'll work to get that done today or Friday.

Edit: I swear I'm working on it! Got side tracked by publishing core notes and stuff but I am drafting.

Edit: 2 mo. later it is obvious I dropped the ball. Talking with @pzuraq about getting it moving again.

@cagrimmett
Copy link

I'm a new Ember dev and we use @rwjblue's ember-truth-helpers addon (and I didn't know it was an addon, assumed it was core.) When I spun up a side project app, I was surprised/frustrated when these basic helpers weren't working. Should definitely be part of core! I use it in at least half of my templates.

@mixonic
Copy link
Member

mixonic commented Dec 8, 2019

Framework core discussed this at the September F2F: https://github.com/emberjs/core-notes/blob/master/ember.js/2019-09/F2F_Ember_Core_Team_20190915.md#basic-helpers-rfc

I'll try to summarize: @chancancode left a string of comments above, and he raised a few different points which we think remain unaddressed. That said, I think comments were a bit ad-hoc in their presentation and the F2F conversation may help clarify the over-arching concerns.

My goal in raising the discussion at F2F was to try and find a path forward, and I think everyone is aligned on that goal. We all want to see some more convenience helpers added to the core framework.

HBS v. JS truthiness, {{and}} {{or}} {{not}}

@chancancode raised the questions of "What truthiness semantics should these helpers (at least and, or, not) assume?" here. Just below @cibernox explained "eq behaves like === and not-eq behaves like !==, just like in ember-truth-helpers."

The problem of adopting JS semantics is that HBS already has truthiness semantics of its own, at least of some of the cases here. Where possible we would prefer to base additional helpers on current semantics. This helps the template language maintain cohesion and allows to us implement more complex helper atop simple tools.

For example instead of defining (and) by deferring to JS && semantics as currently suggested it should be defined via HBS semantics:

{{and a}} is an expression of {{if a a false}}
{{and a b}} is an expression of {{if a (if b b false) false}}
{{and a b c}} is an expression of {{if a (if (if c c false) false) false}}

By expressing the design as HBS we can avoid needing to consider differences in how HBS and JS may decide truthiness, and the question of "what value is returned in what case" becomes explicit. The glimmer engine can additionally implement optimizations like shortcutting evaluation of subexpressions as a VM optimization instead of deferring to JS.

Of course just like JS truthiness can be subtle and sometimes confusing, HBS truthiness is probably the same. Some notable warts are empty array behaviors and the difference in the behavior of a blank string vs. a blank SafeString. Any proposal for building on (if should at least enumerate the current behaviors, but probably should consider changing those behaviors out of scope.

This is also trivially possible with (not):

{{not a}} is an expression of {{if a false true}}

{{or}} is a bit more complex but still possible:

{{or a}} is an expression of {{if a a false}}
{{or a b}} is an expression of {{if a a (if b b false)}}
{{or a b c}} is an expression of {{if a a (if b b (if c c false))}}

Given that these are a reasonable design compromise (which I expect they are?) we can look at more meaty challenges. I think a stand-alone RFC for (and) (or) (not) based on HBS semantics would be easy to advance.

{{eq}} {{neq}}

We spend some time at the F2F thinking about if HBS already has equality semantics. The consensus answer is "probably not", although there was some suggestion you could consider the {{#each}} DOM stability semantics as being dependent on some measure of value equality.

After discussion there was some loose consensus that === in JS would be appropriate for template equality semantics.

An open issue was the equality of safe strings. Does {{eq htmlSafeStringOfFoo anotherHtmlSafeStringOfFoo}}? Since creating a safe string creates an object a === comparison would have two safe strings from the same actual string fail an equality comparison.

{{neq}} could be implemented atop the {{eq}} semantics as {{if (eq a b) false true}}

{{lt}} {{gt}}

These helpers also require a definition. I think there is some amount of discussion possible around if non-numeric values should ever be permitted as arguments. In JS 'abc' > NaN === false. Does that make any sense? Maybe we should not lean on the JS versions of these operators as we build the HBS versions.

How to move forward

My own proposal to get work landed in the quickest manner and to keep the conversations focused is that this work be split into 3 RFCs.

  • RFC: Adding Logical Operators to Templates ({{and}} {{or}} {{not}})
    • This should enumerate examples of what is truthy and falsy in the current implementation
  • RFC: Adding Equality Operators to Templates ({{eq}} {{neq}})
  • RFC: Adding Numerical Comparison Operators to Templates ({{lt}} {{gt}})

This is only my suggestion, as I think the logical operator RFC would be trivial where the other two will benefit from being more focused. At the least I know it would aid framework core in having discussion and moving each forward to consensus.

@cibernox
Copy link
Contributor Author

cibernox commented Dec 8, 2019

Great to see that this can finally move. My only concern with those helpers obeying handlebars' trutiness semantics is that it is different from what ember-truth-helpers do.

We'd have to plan to allow people to migrate without too much pain. I'm particularly worried about addons that use ember-truth-helpers, not so much about application code.

I agree that splitting this RFC into three RFCs makes things easier. The one for the equality operators a the one for numerical comparisons are less controversial as they would be identical to those in Ember-truth-helpers and can be implemented quickly.

I'll try to write all/some of those RFCs this week, but by any means if anyone wants to help, by all means, just call dibs on one of them in this thread.

@cibernox
Copy link
Contributor Author

cibernox commented Dec 8, 2019

@mixonic I've open three separate RFCs as suggested, feel free to close this one now.

@rwjblue
Copy link
Member

rwjblue commented Dec 20, 2019

Thanks @cibernox! We discussed this in todays core team meeting and are moving this RFC into "FCP to Close" (in favor of the broken out / smaller RFCs).

@rwjblue rwjblue added the FCP to close The core-team that owns this RFC has moved that this RFC be closed. label Dec 20, 2019
@Panman82
Copy link

Panman82 commented Dec 20, 2019

Any thoughts on where a {{coalesce}} helper might best fit in the separated RFCs? Or maybe it's own RFC? Or not at all?? (It was just a discussion I brought up earlier, as a way to provide defaults for glimmer component @args in the template.) I've created a Gist of what I figured it would look like / how it would function: https://gist.github.com/Panman82/04d96915d8854f20ede4a4788fcb11e3

@locks
Copy link
Contributor

locks commented Dec 20, 2019

@Panman82 I would submitted it on its own, as I think it would benefit from focused discussion!

@locks
Copy link
Contributor

locks commented Jan 14, 2020

The FCP period has lapsed without further discussion, so I'll be closing this RFC.
Thanks everyone for participating, see you in the discussions for #560, #561 and #562 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FCP to close The core-team that owns this RFC has moved that this RFC be closed. T-framework RFCs that impact the ember.js library
Projects
None yet
Development

Successfully merging this pull request may close these issues.