[Feat]: equality operators #1277

snewcomer · 2021-02-28T13:38:25Z

RFC: emberjs/rfcs#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. - snewcomer#1 snewcomer#3

rwjblue · 2021-02-28T14:38:50Z

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).

snewcomer · 2021-04-15T00:27:15Z

packages/@glimmer/compiler/lib/passes/1-normalization/keywords/impl.ts

@@ -34,7 +34,8 @@ class KeywordImpl<
  constructor(
    protected keyword: S,
    type: KeywordType,
-    private delegate: KeywordDelegate<KeywordMatches[K], Param, Out>
+    private delegate: KeywordDelegate<KeywordMatches[K], Param, Out>,
+    private options?: { strictOnly: boolean }


Wasn't sure if this anonymous type was ok with you. (we could share an actual interface in multiple spots)

rwjblue · 2021-04-26T20:35:52Z

packages/@glimmer/compiler/lib/passes/1-normalization/keywords/utils/equality.ts

+function assertEqualKeyword(node: GenericKeywordNode): Result<ASTv2.PositionalArguments> {
+  let {
+    args: { named, positional },
+  } = node;
+
+  if (named && !named.isEmpty()) {
+    return Err(generateSyntaxError(`(eq) does not take any named arguments`, node.loc));
+  }
+
+  return Ok(positional);
+}


I think we also need to assert if you use more than two positional arguments

https://github.com/emberjs/rfcs/blob/master/text/0560-add-equality-operators.md#eq

{{eq}}
Binary operation. Throws an error if not called with exactly two arguments. Equivalent of === This is identical to the eq helper in ember-truth-helpers

rwjblue · 2021-04-26T20:36:55Z

packages/@glimmer/compiler/lib/passes/1-normalization/keywords/utils/equality.ts

+function assertNotEqualKeyword(node: GenericKeywordNode): Result<ASTv2.PositionalArguments> {
+  let {
+    args: { named, positional },
+  } = node;
+
+  if (named && !named.isEmpty()) {
+    return Err(generateSyntaxError(`(neq) does not take any named arguments`, node.loc));
+  }
+
+  return Ok(positional);
+}


Needs to assert RE: more than two positional params

{{neq}}
Binary operation. Throws an error if not called with exactly two arguments. Equivalent of !== This is identical to the not-eq helper in ember-truth-helpers, except for the name.

rwjblue

I think we also need to update https://github.com/glimmerjs/glimmer-vm/blob/master/packages/@glimmer/vm/lib/opcodes.toml for these new opcodes.

rwjblue · 2021-04-26T20:38:35Z

packages/@glimmer/integration-tests/test/keywords/equality-test.ts

+
+  @test
+  ['it works multiple arguments eq']() {
+    const AComponent = defineComponent({}, '{{eq 1 1 1}}');


I think this is not supposed to work

emberjs/rfcs#560 (comment)

@rwjblue easy enough to support multiple I imagine. What do you think?

packages/@glimmer/runtime/lib/compiled/opcodes/expressions.ts

MelSumner · 2021-07-30T16:41:18Z

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!

pzuraq · 2021-07-30T18:57:55Z

@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.

MelSumner · 2021-07-30T18:58:44Z

@pzuraq perfect, thanks for clarifying!

snewcomer · 2021-08-01T13:51:31Z

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? - emberjs/rfcs#560 (comment)

NullVoxPopuli · 2022-01-27T04:07:37Z

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

snewcomer · 2022-01-27T15:50:39Z

@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...

NullVoxPopuli · 2022-01-27T16:08:31Z

I'll ping them

chancancode · 2022-02-11T23:59:39Z

@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.

NullVoxPopuli · 2022-02-14T21:39:43Z

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

snewcomer · 2022-02-15T14:39:23Z

Sounds like we have a few steps.

Just to note, there is a leaf PR that would need to be merged - [Feat]: logical keywords snewcomer/glimmer-vm#3. I could merge them all down to this branch if we are generally comfortable with the implementation. Sounds like we might be but...
Pending wycats comments
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.

chancancode · 2022-02-15T15:26:37Z

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

snewcomer · 2022-02-21T04:40:14Z

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

bertdeblock

Looking forward to this 👍

bertdeblock · 2022-03-15T16:23:43Z

packages/@glimmer/compiler/lib/passes/1-normalization/keywords/utils/equality.ts

+    return Err(
+      generateSyntaxError(
+        `(eq) must receive two positional parameters. Received ${
+          positional?.size ?? 0


Suggested change

positional?.size ?? 0

positional.size ?? 0

Since we already do positional.size on line 18?

bertdeblock · 2022-03-15T16:24:03Z

packages/@glimmer/compiler/lib/passes/1-normalization/keywords/utils/equality.ts

+    return Err(
+      generateSyntaxError(
+        `(neq) must receive two positional parameters. Received ${
+          positional?.size ?? 0


Suggested change

positional?.size ?? 0

positional.size ?? 0

Since we already do positional.size on line 59?

snewcomer force-pushed the sn/equality-operators branch from df5c627 to a40dc36 Compare February 28, 2021 13:44

This was referenced Mar 2, 2021

[Feat]: comparison operators snewcomer/glimmer-vm#1

Open

Logical helpers renaming emberjs/rfcs#714

Closed

snewcomer force-pushed the sn/equality-operators branch from a40dc36 to be501f0 Compare April 13, 2021 03:53

snewcomer commented Apr 15, 2021

View reviewed changes

snewcomer force-pushed the sn/equality-operators branch from aa76473 to 7710fc0 Compare April 26, 2021 20:22

rwjblue requested changes Apr 26, 2021

View reviewed changes

snewcomer requested a review from rwjblue April 27, 2021 19:18

NullVoxPopuli approved these changes Apr 30, 2021

View reviewed changes

snewcomer force-pushed the sn/equality-operators branch from 656676e to 3e0a222 Compare August 10, 2021 13:40

snewcomer force-pushed the sn/equality-operators branch from 3e0a222 to 75a7746 Compare November 18, 2021 16:11

snewcomer mentioned this pull request Jan 29, 2022

[Feat]: element keyword #1313

Open

5 tasks

snewcomer added 6 commits February 15, 2022 08:33

[Feat]: equality operators

fa8bfcb

Strict only helpers

cba1602

some progress with strictMode in tests

61d5d74

proper re-render tests

86aaf17

pop and push instead of popJs and pushJs

518b33e

add opcodes to toml file

c4948ec

snewcomer force-pushed the sn/equality-operators branch from 75a7746 to c4948ec Compare February 15, 2022 14:34

fail on anything but two arguments

dec2734

bertdeblock reviewed Mar 15, 2022

View reviewed changes

[WIP] add loose mode compatible keywords

15dbe96

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feat]: equality operators #1277

[Feat]: equality operators #1277

snewcomer commented Feb 28, 2021 •

edited

Loading

rwjblue commented Feb 28, 2021

snewcomer Apr 15, 2021 •

edited

Loading

rwjblue Apr 26, 2021

rwjblue Apr 26, 2021

rwjblue left a comment

rwjblue Apr 26, 2021

snewcomer Jan 14, 2022

MelSumner commented Jul 30, 2021

pzuraq commented Jul 30, 2021

MelSumner commented Jul 30, 2021

snewcomer commented Aug 1, 2021 •

edited

Loading

NullVoxPopuli commented Jan 27, 2022

snewcomer commented Jan 27, 2022

NullVoxPopuli commented Jan 27, 2022

chancancode commented Feb 11, 2022

NullVoxPopuli commented Feb 14, 2022

snewcomer commented Feb 15, 2022

chancancode commented Feb 15, 2022

snewcomer commented Feb 21, 2022

bertdeblock left a comment

bertdeblock Mar 15, 2022

bertdeblock Mar 15, 2022

[Feat]: equality operators #1277

Are you sure you want to change the base?

[Feat]: equality operators #1277

Conversation

snewcomer commented Feb 28, 2021 • edited Loading

rwjblue commented Feb 28, 2021

snewcomer Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

rwjblue Apr 26, 2021

Choose a reason for hiding this comment

rwjblue Apr 26, 2021

Choose a reason for hiding this comment

rwjblue left a comment

Choose a reason for hiding this comment

rwjblue Apr 26, 2021

Choose a reason for hiding this comment

snewcomer Jan 14, 2022

Choose a reason for hiding this comment

MelSumner commented Jul 30, 2021

pzuraq commented Jul 30, 2021

MelSumner commented Jul 30, 2021

snewcomer commented Aug 1, 2021 • edited Loading

NullVoxPopuli commented Jan 27, 2022

snewcomer commented Jan 27, 2022

NullVoxPopuli commented Jan 27, 2022

chancancode commented Feb 11, 2022

NullVoxPopuli commented Feb 14, 2022

snewcomer commented Feb 15, 2022

chancancode commented Feb 15, 2022

snewcomer commented Feb 21, 2022

bertdeblock left a comment

Choose a reason for hiding this comment

bertdeblock Mar 15, 2022

Choose a reason for hiding this comment

bertdeblock Mar 15, 2022

Choose a reason for hiding this comment

snewcomer commented Feb 28, 2021 •

edited

Loading

snewcomer Apr 15, 2021 •

edited

Loading

snewcomer commented Aug 1, 2021 •

edited

Loading