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

Adding Logical Operators to Templates #562

Merged
merged 6 commits into from
Jan 15, 2021

Conversation

cibernox
Copy link
Contributor

@cibernox cibernox commented Dec 8, 2019

Extracted from RFC #388

Rendered


## Unresolved questions

- Consider following Javascript's definition of truthiness. This would also help with the transition from `ember-truth-helpers`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably belongs under “Alternatives” instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd argue that it belongs here. The way I understand RFCs, alternatives is where you list alternative options to implementing the RFC, not so much about doubts regarding what the best implementation is.

If anyone can confirm that I got it wrong, I'm happy to change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think an exact implementation of ember-truth-helpers logical operators (except maybe not) is not what anyone wants, given the desire for short circuit logic. But alternatives sections are not always fully specced out, so I think with just a little more detail, you could write an alternative.

Copy link

Choose a reason for hiding this comment

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

Being someone who believes that the lack of logical operators is the main benefit that handlebars provides, and one who has never had a need to install this addon in the many ember apps i have built, simply assuming that this is something that is required to be installed does not sit well with me. I would like somewhere in this process to highlight the benefits that not having these be a part of the framework provides.

Copy link
Contributor Author

@cibernox cibernox Dec 10, 2019

Choose a reason for hiding this comment

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

@webark You can certainly live without those helpers, but I'll add some examples of things that are much nicer with helpers like those:

Default Values in template-only components

Imagine a template only component that wants to have a default value for say, @name

<img class="avatar" src={{or @user.avatar "/imgs/default-avatar.png">

Perform an operation on each iteration of a loop without extracting another component

{{#each @users as |user|}}
  Name: {{user.name}} 
  {{#if (and session.hasAdminPrivileges user.inactive)}}
    <button {{on "click" this.activate}}>Delete</button>
  {{/if}}
{{/each}}

In both situation you can find a way to do the same things without using any of those helpers, but it involve either create a JS file for a component only to put a computed property on it, or to extract a new component to call it on each iteration of a loop, just to perform a very simple logical operation.

It's a matter of convenience and as any tool it can be abused, but the popularity of ember-truth-helpers proves that devs really find it convenient.

I think that Ember providing just the most basic and used ones (and, or, not, eq are the main 4 for me, but this RFC focuses on the first 3) and optimized at the VM level we are alleviating a pain.

Copy link

Choose a reason for hiding this comment

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

call me a curmudgeon (and i’ll postpone my “at this point we might as well be using jsx” rant for another day) I just feel it should be mentioned why this was never in core to begin with. Your examples, to me, are the base examples of why. Just a brief “handlebars was originally meant to be “logic-less” but it has grown, and the community has pushed it, past that for sometime now” blurb would be helpful, at least for a historical sense.

@Gaurav0
Copy link
Contributor

Gaurav0 commented Dec 10, 2019

Being someone who believes that the lack of logical operators is the main benefit that handlebars provides, and one who has never had a need to install this addon in the many ember apps i have built, simply assuming that this is something that is required to be installed does not sit well with me. I would like somewhere in this process to highlight the benefits that not having these be a part of the framework provides.

I think this needs more discussion in the other helpers, but for these particular helpers, we can't build short circuit versions without direct core support. Would it be possible to build some primitives to make short circuiting possible while keeping the helpers (and actual semantics) in an addon?

@cibernox
Copy link
Contributor Author

I think this needs more discussion in the other helpers, but for these particular helpers, we can't build short circuit versions without direct core support. Would it be possible to build some primitives to make short circuiting possible while keeping the helpers (and actual semantics) in an addon?

@Gaurav0 I'll add it as a note, but even if it was possible and short-circuiting was possible in user-land, I still think those helpers are useful regardless of any optimization we may add, so I don't want to focus the RFC on the possible performance improvements.

Probably on the big picture the perf improvements we may squeeze from short-circuiting will be rather small in anything but pretty contrived examples.

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! 🙏

@locks locks self-assigned this Feb 26, 2020
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.

We discussed this at todays core team meeting, and the team broadly agreed with the detailed design here. However, we believe that we need to flesh out the how we teach this a bit more (I left an inline comment with some reasoning here). I think with some tweaks there to address that concern, we should be able to move forward here.

Comment on lines 78 to 83
## How we teach this

The introduction of these helpers does not impact the current mental model for Ember applications.

In addition to API and Guides documentation with illustrative examples of usage of the various helpers, stressing out
where the definition of truthiness of handlebars is different from the one in Javascript.
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to beef this section a bit more. Specifically, I do think this changes some of the mental models. For example, commonly we punt folks into JS-land instead of using ember-truth-helpers in the guides/tutorial (e.g. we might suggest using computed.not). This would definitely change if we introduce these helpers.

@cibernox
Copy link
Contributor Author

cibernox commented Mar 1, 2020

@rwjblue I updated the How we teach section, check it out.

@simonihmig
Copy link
Contributor

simonihmig commented Nov 22, 2020

Any updates on this? Given that the need for these helpers is quite ubiquitous I believe and trivial to implement (except for the short-circuiting maybe), advancing this RFC seems like a very low-hanging fruit to me!

I would like to add one thing though: has another helper like {{default}} been considered? It might not be truly a "truth helper", but {{or}} is also not really used as a boolean logic operator in most cases, but rather as a way to provide a default value, as in @cibernox's previous example:

<img class="avatar" src={{or @user.avatar "/imgs/default-avatar.png"}}>

Tbh, I don't like this pattern that much, because

  • it does not clearly express the intent, as this is not about some boolean operation that returns a boolean. Rather it relies on JavaScript's loosely typed interpretation of boolean logic, which in this case would return the second argument if the first evaluates to falsy
  • given that loosely typed behavior, this happens to work for string values as in the example above, but fails miserably for other values that should be considered valid values (i.e. which should not get overridden by the default value) but evaluate to falsy (false, 0, ""). Take this example:
<Modal @open={{or @modalOpen true}}/>

(The modal would always be open here, no matter what you pass as @modalOpen)

That's why I am so happy about the nullish coalescing (??) operator that we have now in JS-land, as it handles both these concerns (intent and behavior) much better than ||.

Side node: you see I am not a fan of JavaScript's loose interpretation of boolean logic in general. Maybe because of the stronger mathematical education I enjoyed in school and university. I just find that boolean logic is soo easy to reason about when constrained to functions that take one or more booleans and return a boolean (truth tables!). As it was supposed to be! But JS semantics are obviously not going to change. But let's move on, and sorry for my slightly off-topic rant here! 😆

So I would propose to add something like {{default}} (we can argue about naming of course), which is implemented using ??, just as {{or}} would basically use || (except for the slightly different handlebars semantics of truthiness).

You could argue that this can be added in its own RFC, however I would argue that once you add {{or}} to Ember's core, it would perpetuate that use (IMHO misuse), and it would be hard to roll it back later.

@rwjblue
Copy link
Member

rwjblue commented Jan 8, 2021

We chatted about this in todays core team meeting, and are in favor of moving forward here.

One thing to call out explicitly, this RFC does not propose import paths (which helpers would require see the strict mode RFC for details). At the time of authoring, that distinction was not present (which is why it isn't discussed here), but after discussion with the core team we think that making these keywords (which do not need imports) is the correct path anyways (therefore no changes are required to the RFC).

Additionally, landing this in Ember will require a small deprecation when using ember-truth-helpers's and/or/not (since the semantics are subtlely different). In order to preserve the correct behavior in your application, Ember will use a resolved helper for these names if one is found (along with a deprecation).


All of that being said, we are moving this into final comment period (finally 😉).

@cibernox
Copy link
Contributor Author

cibernox commented Jan 8, 2021

Great. I think that making them keywords (so they don't have to be imported all the time) makes sense for such basic operators. Let's have it merged!

@jelhan
Copy link
Contributor

jelhan commented Jan 9, 2021

The RFC proposes that {{and}} and {{or}} keywords do not return a boolean but the value of one of its arguments. I don't think that's a good design choice.

The main motivation that I have seen for returning the value rather than a boolean is using {{or}} to define default values. I think @simonihmig raised valid concerns with that pattern. Additionally there is another RFC, Argument Default Primitives (#695), since two weeks, which addresses the same problem. There wouldn't be any need to use {{or}} for default values in templates if Argument Default Primitives or something similar lands.

I fear not returning a boolean from keywords called {{and}} and {{or}} would cause confusion and will cause bugs. Especially if the keywords are not used with {{if}} or {{unless}} but to pass values to components, helpers or modifiers.

Let's assume a <Foo> component with a @show argument, which takes a boolean. Let's further assume that a consumer invokes it like this: <Foo @show={{and (array)}} />. Let's see what could go wrong depending on implementation details of <Foo>:

  1. The component could validate the argument and throw because @show is not a boolean but an array. 💥 If the Argument Validation Primitives RFC (Argument Validation Primitives #694) lands these kind of argument validation may get way more common that it is today.
  2. The component consumes the value without validating it. Depending if consumes it in the template or in its JavaScript class it would be either considerd falsy or truthy . 😱

A consumer would need to cast the value returned by {{and}} and {{or}} to boolean to avoid these problems. So the correct invocation would look like this: <Foo @show={{not (not (and (array)))}} />. 🤯

@chriskrycho
Copy link
Contributor

@jelhan these are all wholly valid points… but they’re also fundamental to how JavaScript works, and one of the things I see quite consistently as a challenge for folks working with Glimmer/Handlebars templates vs. working in React, Svelte, etc. is the points where the semantics of these kinds of things differ. They're surprising, and needlessly so, at least in context.

let value = 1 || false; // 1
let flipped = false || 1; // 1
let anded = 1 && 2; // 2
let flipThat = 2 && 1; // 2

etc.

If we give {{and}} and {{or}} different semantics on this front in templates than in JavaScript, it increases the teaching burden and removes symmetry between these helpers and any such helpers you would implement yourself. In particular, given a world of template imports (🔜!), you can imagine someone being confused why the built-in version and the version they might implement themselves like this (with a hand-wave syntax) have different semantics:

function or([a, b]) {
  return a || b;
});

export default<template>
  <div>{{or @something this.someOtherValue}}</div>
</template>

More/worse, there are times when you might need to switch from the built-ins to something slightly more complicated, and then you have to keep the differences in your head: refactoring from {{or}} to something which uses JS || is no longer straightforward but can have (sometimes wildly!) different semantics. The combination of increased burden on teaching (esp. for folks coming from other modern frameworks) and of ongoing things to keep in your head both point pretty conclusively in the direction of matching JS’s semantics here, even though JS’s own semantics are a mess.

@chancancode
Copy link
Member

Let's assume a component with a @show argument, which takes a boolean. Let's further assume that a consumer invokes it like this: <Foo @show={{and (array)}} />.

This is indeed not how it should be used and too complicated anyway. If you want strict Boolean values, you can just do <Foo @show={{if array true false}} />.

@knownasilya
Copy link
Contributor

The missing hole is {{eq}} that's the other one I use most.

@jelhan
Copy link
Contributor

jelhan commented Jan 10, 2021

@chriskrycho Thanks a lot for explaining the motivation. Wasn't clear to me that it should work as || and && in JavaScript. But actually that makes sense.

I guess definition of truthiness is only exception for consistency within the template? To avoid a situation in which {{and}} consider an empty array as truthy which is considered falsy by {{if}}?

@chrism
Copy link

chrism commented Jan 13, 2021

The missing hole is {{eq}} that's the other one I use most.

Honestly, I think its a great idea to include all these by default but this is the same issue for me.

Usually I add ember-truth-helpers in large part because {{eq}} is so useful.

So that means I would still probably need to add it anyway and then this difference in behaviour...

This is NOT equivalent to the {{not}} helper from ember-truth-helpers because unlike this proposed helper, the one in ember-truth-helpers uses Javascript's definition of truthiness.

I guess would change subtlety and so require some care not to introduce bugs after the install?

@rwjblue
Copy link
Member

rwjblue commented Jan 13, 2021

The missing hole is {{eq}} that's the other one I use most.

Ya, that is being proposed in #560. Hopefully we can finalize our thoughts there (I made a comment explaining the current mental state of the core team recently) and release these two features roughly together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.