[Glimmer2] Implement positional parameter

[Serabe]

This PR implements positional parameters in Glimmer 2.

• Basic processing of positional parameters
• Query parameters in dynamic components ({{component "x-component" "a" "b"}}`)
• Improve processing based on Glimmer 2 positional args API.
• Pass all tests that currently pass in @htmlbars.
• If id arg is implemented before closing this PR, change tests back to id="x".

[chancancode]

@Serabe I pushed a change in Glimmer for the component helper case in glimmerjs/glimmer-vm@2b66088

The corresponding Ember change would be something like this (basically what I did in the Glimmer test):

diff --git a/packages/ember-glimmer/lib/components/dynamic-component.js b/packages/ember-glimmer/lib/components/dynamic-component.js
index 58dd886..e8a8fa0 100644
--- a/packages/ember-glimmer/lib/components/dynamic-component.js
+++ b/packages/ember-glimmer/lib/components/dynamic-component.js
@@ -1,10 +1,13 @@
-import { StatementSyntax } from 'glimmer-runtime';
+import { ArgsSyntax, StatementSyntax } from 'glimmer-runtime';

 export class DynamicComponentSyntax extends StatementSyntax {
   constructor({ args, templates }) {
     super();
-    this.args = args;
-    this.definition = dynamicComponentFor;
+    this.definition = {
+      args: ArgsSyntax.fromPositionalArgs(args.positional.slice(0, 1)),
+      factory: dynamicComponentFor
+    };
+    this.args = ArgsSyntax.build(args.positional.slice(1), args.named);
     this.templates = templates;
     this.shadow = null;
   }
diff --git a/packages/ember-glimmer/lib/components/outlet.js b/packages/ember-glimmer/lib/components/outlet.js
index 784237a..a07020f 100644
--- a/packages/ember-glimmer/lib/components/outlet.js
+++ b/packages/ember-glimmer/lib/components/outlet.js
@@ -1,4 +1,4 @@
-import { StatementSyntax } from 'glimmer-runtime';
+import { ArgsSyntax, StatementSyntax } from 'glimmer-runtime';
 import { ConstReference } from 'glimmer-reference';
 import { generateGuid, guidFor } from 'ember-metal/utils';
 import { RootReference, NULL_REFERENCE } from '../utils/references';
@@ -6,8 +6,8 @@ import { RootReference, NULL_REFERENCE } from '../utils/references';
 export class OutletSyntax extends StatementSyntax {
   constructor({ args }) {
     super();
-    this.args = args;
-    this.definition = outletComponentFor;
+    this.definition = { args, factory: outletComponentFor };
+    this.args = ArgsSyntax.empty();
     this.templates = null;
     this.shadow = null;
   }

[chancancode]

re: above – should just work now with #13405 merged

[Serabe]

There are still tests not passing for #13158. I'm trying to look at what the problem might be. Since I'll need it for the contextual parameters part as well, what is the equivalent point to ember-htmlbars/lib/keywors/component.js in Glimmer?

[chancancode]

It'd be in the dynamic component file.

[homu]

☔ The latest upstream changes (presumably #13168) made this pull request unmergeable. Please resolve the merge conflicts.

[chadhietala]

@Serabe Do you have time to take this over line? Otherwise I can take it on.

[Serabe]

Yep. I'll try to do so this weekend.

[Serabe]

I had to change a couple of things. I had to do the operation in two steps, since passing props to an action makes JSON.stringify fail because it detects a circular reference, not so sure why.

I also separated a couple of dynamic component test that were mixed in other tests in curly components.

[chadhietala]

Maybe just return args.positional here instead of allocating the new array?

[Serabe]

args.positional are the values of the positional parameters but we are looking for an array of keys to pass to attrsToProps.

[chadhietala]

Just the small suggestion. Otherwise this LGTM.

[chancancode]

processAttrs and attrsToProps have basically lost their meaning at this point 😨

Can you extract them into your new file in utils and make it something like let { attrs, props } = processArgs(args, klass.positionalParams);?

attrs is the thing we pass to the did*Attrs hooks, and props is what we set on the component instance.

I don't think there is necessarily value of keeping them as separate functions anymore, and passing keys etc. The reason it was factored that way was largely an evolution of the requirements (and for efficiency, like avoiding an Object.keys if we already know the keys form the args) so far, and the new requirements you have introduced here have change the equation significantly.

I think it could have been much better factored if you weren't subject to these random historical constraints. 😄

[Serabe]

Thank you. Updated!

[homu]

☔ The latest upstream changes (presumably #13475) made this pull request unmergeable. Please resolve the merge conflicts.

[homu]

☔ The latest upstream changes (presumably #13490) made this pull request unmergeable. Please resolve the merge conflicts.

[chancancode]

Merged in #13940, thanks @Serabe 👍👍👍