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 Equality Operators to Templates #560

Merged
merged 4 commits into from
Jan 29, 2021

Conversation

cibernox
Copy link
Contributor

@cibernox cibernox commented Dec 8, 2019

Extracted from RFC #388

Rendered

@balinterdi
Copy link

I'll admit it's not very constructive feedback, but can we call it not-eq instead of neq?

@cibernox
Copy link
Contributor Author

cibernox commented Dec 8, 2019

@balinterdi It's one of the points in the Unresolved questions section. Let's someone from core chime in and arguments the reasons to prefer {{neq}} instead of {{not-eq}}. If there was none, I'd also prefer {{not-eq}} because the transition from ember-truth-helpers would be super easy.

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

- If an app already use `ember-truth-helpers`, the `{{eq}}` helper will conflict with the one proposed here. How do we
update `ember-truth-helpers` to make sure the helper of the same name doesn't collide with the built-in one?
- The inequality helper proposed in this RFC is `{{neq}}` while the one in ember-truth-helpers is `{{not-eq}}`. It is
worth considering the benefits that keeping the same name might have in helping apps and addon migrate to the built-in helper.
Copy link
Member

Choose a reason for hiding this comment

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

this could easily be migrated using a template codemod, so probably not much of an issue in reality :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but nothing beats not having to change anything 😄

Choose a reason for hiding this comment

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

@Turbo87 And you have to admit: neq looks way worse than not-eq.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, one could argue that (not (eq foo bar)) is not much longer than (not-eq foo bar) 😅

Choose a reason for hiding this comment

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

Fair enough, I'm happy with (not (eq ... )), too. I don't think gaining 3 characters (neq vs. not-eq) justifies introducing a so-far unseen neq but I might just have a visceral reaction to neq that I have to overcome.

@driesdl
Copy link

driesdl commented Jan 2, 2020

Why not add all truth-helpers, but just these two?

@Turbo87
Copy link
Member

Turbo87 commented Jan 2, 2020

@Driezzz because these are the most uncontroversial

@locks locks self-assigned this Feb 26, 2020
Add `{{eq}}` and `{{neq}}` helpers.

#### `{{eq}}`
Binary operation. Throws an error if not called with exactly two arguments.
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 should allow many arguments with eq, behaving like:

a === b === c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwjblue is that consensuated with the core team? I'm fine either way, I just want to be sure before updating it.

text/0000-add-equality-operators.md Outdated Show resolved Hide resolved
@rwjblue
Copy link
Member

rwjblue commented Jan 8, 2021

Accidentally commented 😩 here instead of #562 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 😉).

@rwjblue
Copy link
Member

rwjblue commented Jan 8, 2021

Dangit! I meant to make that comment over on #562 not this one. 🤦

@rwjblue
Copy link
Member

rwjblue commented Jan 9, 2021

Note: we did talk about this RFC quite a bit also, and I do think that we want to move forward soon. There are just a few other angles that we wanted to talk through (e.g. should there be a way to customize equality or not vs "simple" ===, or if you have a string should we use locale compare instead of ===). Hopefully we can solidify that discussion in the next core team meeting...

@Turbo87
Copy link
Member

Turbo87 commented Jan 9, 2021

IMHO it's complicated enough to explain that empty arrays in HBS are falsy. if equality was also redefined to work different from == and === it would make it even harder to explain to new devs.

@rwjblue
Copy link
Member

rwjblue commented Jan 15, 2021

We discussed this a bit more this week, and are still very much in favor of moving forward.

IMHO it's complicated enough to explain that empty arrays in HBS are falsy. if equality was also redefined to work different from == and === it would make it even harder to explain to new devs.

Agreed. I think this is well stated, and when we discussed it more at todays meeting we shared your conclusion.


Remaining questions that we want to resolve (either in a GlimmerVM team meeting or the next core team meeting):

  • Should we use Object.is instead of ===? The main difference between Object.is and === is comparison with +0 or -0 and NaN (which are all pretty WAT in normal JavaScript). Most folks have no clue that Map/Set/WeakMap don't use === (they use Object.is), and it doesn't cause any issue.
  • Do we have to consider that two helper invocations are guaranteed to be eq to each other? In other words, if we have {{#let this.whatever as |thing|}}{{if (eq thing thing)}}{{/let}}; should the rendering engine even run JavaScript code to determine these are equal or should it assume that they are eq (aka stable) and therefore we can optimize the rendering engine output to avoid the extra comparison. 🤔 (personal opinion note: I think that this may well be true, fine, and awesome; but doesn't have to change anything about this RFC...)

Hopefully, we'll be able to resolve those two issues at the next round of meetings.

@cibernox
Copy link
Contributor Author

cibernox commented Jan 15, 2021

Do we have to consider that two helper invocations are guaranteed to be eq to each other? In other words, if we have {{#let this.whatever as |thing|}}{{if (eq thing thing)}}{{/let}}; should the rendering engine even run JavaScript code to determine these are equal or should it assume that they are eq (aka stable) and therefore we can optimize the rendering engine output to avoid the extra comparison. 🤔 (personal opinion note: I think that this may well be true, fine, and awesome; but doesn't have to change anything about this RFC...)

I think this might be overthinking it. First, IDK if helpers are guaranteed to have stable outputs, at least with class-based-helpers I believe you could perfectly not be the case. Trying to be that smart just to remove a == operation (or Object.is)
seems overkill.

@rwjblue
Copy link
Member

rwjblue commented Jan 15, 2021

I think this might be overthinking it.

Hehe, I don't disagree with you (see my personal note in that snippet); buuuuuutttt sometimes it's what we do. 😺

@rwjblue
Copy link
Member

rwjblue commented Jan 22, 2021

Thanks @pzuraq for adding that section around object equality scenarios (and some potential gotchas there). We discussed this today, and we are moving this into FCP.

@lifeart
Copy link

lifeart commented Jan 23, 2021

not-eq seems more readable for me, comparing to neq

it require less mental parsing, especially for non native speakers and new developers.

Same as if-not is more readable comparing to unless.

related: glimmerjs/glimmer-vm#1240 (comment)
cc: @pzuraq

@rwjblue
Copy link
Member

rwjblue commented Jan 29, 2021

We discussed this at todays meeting, and are still in favor of moving forward.

@rwjblue rwjblue merged commit 40ed8fb into emberjs:master Jan 29, 2021
@rwjblue
Copy link
Member

rwjblue commented Jan 29, 2021

@lifeart

not-eq seems more readable for me, comparing to neq

We discussed the dasherization (or not) a while back, and had previously decided to go with neq. One of the primary reasons for that was that (at the time) we expected that you might want to use a local JavaScript identifier for the helpers (and in JS identifiers you cna't use - within a variable name).

In retrospect, we decided not to make these helpers require being imported (see #562 (comment) for some discussion RE: keywords vs helpers) and so not-eq would technically be feasible (since it wouldn't be an identifier). However, as we discussed this again today we just don't want to continue re-opening and debating prior points of consensus (this RFC has already languished for quite a while, and it would be very unfortunate to continually delay). I think a follow up RFC proposing not-eq and if-unless would be a great place to discuss this further.

@chriskrycho
Copy link
Contributor

Is there a decision about the import locations for this, #561, and #562 (looking toward strict mode)?

@locks
Copy link
Contributor

locks commented Nov 23, 2021

@chriskrycho I was under the impression these would be part of the "prelude".

@chriskrycho
Copy link
Contributor

I have gotten conflicting impressions at different times! I think it is fine for these (along with things like on and fn) to be in the "prelude", but we should clarify it. Possibly that work just needs to be done as part of the strict mode/template imports work?—in which case I'll include it in that RFC (which I'm much of the way through drafting!).

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.

None yet

9 participants