glimmer-vm icon indicating copy to clipboard operation
glimmer-vm copied to clipboard

Published 20 hours ago • verified publisher icon glimmerjs

[Feat]: equality operators

Open snewcomer opened this issue 3 years ago • 13 comments

RFC: https://github.com/emberjs/rfcs/pull/560

  • eq
  • neq

Related pkg - https://github.com/jmurphyau/ember-truth-helpers/blob/master/addon/helpers/equal.js

  • These keyword helpers will only be enabled in strict mode.

Some outstanding questions:

  • ~~Will loose mode will compile this keyword into a helper, calling the existing (addon such as ember-truth-helpers or other) if present. Should we add a deprecation at this point.~~ feature flag strictOnly added
  • change neq to not-eq. Happy to open an another RFC or amendment to resolve. This would also align us and allow us to deprecate ember-truth-helpers as well.

Other upstream PRs to be merged first are here, starting with logical operators. - https://github.com/snewcomer/glimmer-vm/pull/1 https://github.com/snewcomer/glimmer-vm/pull/3

snewcomer avatar Feb 28 '21 13:02 snewcomer

We need to determine who will "win" in the case a user has ember-truth-helpers installed as well

The resolved helper must win (we can't assume that a resolved helper named eq is from ember-truth-helpers and therefore make assumptions about its internal implementation), but I am not 100% we want to issue a deprecation in all cases where a helper is resolved from user land for one of these names.

and how to throw a deprecation warning.

RE: when to issue a deprecation: I think we would only want to issue a deprecation if our implementation returns a different result than the resolved helper version.

RE: how to issue a deprecation: there is a nice deprecation system already built out, you import the deprecate function from global context and then invoke it. Ember has a hook into those deprecations and can augment the deprecation options (e.g. to add for/since/until/etc).

rwjblue avatar Feb 28 '21 14:02 rwjblue

What else does this PR need to land? I would love love love to see this land. <3 Thank you all for working on it!

MelSumner avatar Jul 30 '21 16:07 MelSumner

@MelSumner part of the reason this has been blocked is because of v4 prep, we didn't want to land more changes in the VM before the last 3.x LTS was fully stable, in case we had to backport more fixes (there have been a few already).

Once that's done, I think the biggest issue is we need to actually change the way this is implemented so that in loose mode the resolved helpers win. That is a bit of a bigger change overall, because IIRC keywords always win in loose mode, so we need to implement these as "fallback helpers". I believe we were talking about doing both implementations, because ultimately we do want these to be keywords, and then having the compiler choose which one to use depending on whether or not the template was in loose mode. Also, the outstanding comments need to be addressed.

pzuraq avatar Jul 30 '21 18:07 pzuraq

@pzuraq perfect, thanks for clarifying!

MelSumner avatar Jul 30 '21 18:07 MelSumner

this is implemented so that in loose mode the resolved helpers win

It has been a while so I may have this wrong but I I think one workaround that we talked about and I put into this PR was a flag strictModeOnly. Then in ember.js, I would implement the flags again (for loose mode?). We can chat on Monday about it though!

RE: comments I believe {{eq 1 1 1}} was asked for in a comment. But happy to change to two params. More than two is easy enough? - https://github.com/emberjs/rfcs/pull/560#discussion_r385882319

snewcomer avatar Aug 01 '21 13:08 snewcomer

What's left here? It's been a couple months

NullVoxPopuli avatar Jan 27 '22 04:01 NullVoxPopuli

@NullVoxPopuli I believe nothing. I'm excited about this but I'll need some help from either @chancancode and/or @rwjblue to review. I've addressed the comments so far...

snewcomer avatar Jan 27 '22 15:01 snewcomer

I'll ping them

NullVoxPopuli avatar Jan 27 '22 16:01 NullVoxPopuli

@wycats has thoughts on the strict vs loose difference (he don't think it is correct that we should have to handle them differently). He'll comment later.

As for neq vs not-eq, that was already considered during the RFC and I would not be inclined to reopen it yet again, especially if we are looking to expedite things.

Regarding >2 arguments for eq, I don't have a problem with it personally, but landing the error now following the RFC and removing it later wouldn't be a breaking change either, that would be the easy thing to do. But since this is already implemented someone could also go ahead an open an amendment PR and try to get it through pretty quickly. Either way is fine with me.

chancancode avatar Feb 11 '22 23:02 chancancode

does anything need to be done? can this be merged?

NullVoxPopuli avatar Feb 14 '22 21:02 NullVoxPopuli

Sounds like we have a few steps.

  1. Just to note, there is a leaf PR that would need to be merged - https://github.com/snewcomer/glimmer-vm/pull/3. I could merge them all down to this branch if we are generally comfortable with the implementation. Sounds like we might be but...
  2. Pending wycats comments
  3. And adding an error for > 2 arguments seems like a safe approach. I'd be happy to add that to this PR unless somebody has a strong feeling.

snewcomer avatar Feb 15 '22 14:02 snewcomer

And adding an error for > 2 arguments seems like a safe approach. I'd be happy to add that to this PR unless somebody has a strong feeling.

Sounds good to me, pretty easy to change/fix later

chancancode avatar Feb 15 '22 15:02 chancancode

This has been updated with strict 2 arguments to eq and neq!

snewcomer avatar Feb 21 '22 04:02 snewcomer