[BREAKING BUGFIX] Fix positional params behavior to match Function#bind.

[rwjblue]

In general, the concept of contextual components (which is what required the creation of (component helper) was meant to simulate "closing over" prior arguments.

In the case of named arguments (aka "hash" arguments) the mechansim for that is fairly simple: merge all provided named arguments from each layer "upwards". This is what the majority of (component usages are relying on heavily today, and it works wonderfully!

How exactly to handle positional arguments was a bit less obvious. The original implementation was to treat each layer as able to overwrite the prior layers positional arguments.

For example, in this example the inner invocation (with 4 5 6 specified) would override the earlier passing of 1 2 3:

{{#with (component 'x-foo' 1 2 3) as |comp|}}
  {{component comp 4 5 6}}
{{/with}}

For the most part, this behavior has not been an issue. Most likely this is due to the fact that positionalParams with components are used fairly rarely, and when they are folks haven't noticed/used this clobbering behavior.

This commit changes the positional param behavior with contextual components to more closely match how Function.prototype.bind works (and more closely matches folks mental model of "closing over").

After these changes the snippet above would result in invoking x-foo with 1 2 3 4 5 6.

As you can see, this now more closely resembles closing over the earlier values, and matches how the action helper handles positional parameters.

Unfortunately, this bug fix is also a breaking change to folks that have been using contextual components that have postionalParams set and are utilizing the clobbering behavior.

[cibernox]

@rwjblue While I think that "pushing" newer params at the end of the list is the most intuitive behaviour, would you consider concatenating arguments in the reverse order (in your example [4,5,6].concat([1,2,3])) as less breaking, as if users are using the "replace previous params" feature this would retain those passed arguments and add the old ones at the end of the array.

That said, I'm happy with a small breaking change for a simpler mental model, it just seemed a relevant thought.

[rwjblue]

would you consider concatenating arguments in the reverse order (in your example [4,5,6].concat([1,2,3])) as less breaking

I agree that it would be less breaking, but it doesn't actually satisfy the real reasons for the change.

• The divergence from the concept of "currying" or "closures" in the current system is very hard to reason about.
• The divergence from how a very similar concept is managed with actions.
• The inability to compose / wrap positional params at various layers.

[cibernox]

@rwjblue You're preaching to the choir here, but I thought it was valuable to mention the alternative. I also thing that currying is the right behaviour.

[rwjblue]

@Serabe - You mentioned in slack that you had some concerns with the change. Mind chiming in?

[Serabe]

The problem I see with concatenating positional parameters is that they do not really exist, they are but a trick. There are two types of positional parameters:

• Named positional parameters: in this case, the positionalParameters is an array of property names to assign the values to.
• Rest positional parameters: in this case, a property gets all the positional parameters in an array.

In any case, this assignment is done transparently, the component knows nothing about positional or named arguments (this is not actually true after Glimmer 2 if I remember correctly, but it is still true in userland I think).

Furthermore, assigning a property via a positional parameter and a named argument at the same time would throw an error.

OTOH, we have closure actions that, in addition to closing over something, curry their arguments (closing over something and currying being two completely different things). If we are doing this just because closure actions curry their args, I think it should not be done. Two different features can have different behaviours and, as I said before, closing over is different than currying the arguments.

I think current implementation has a simpler mental model since it made both positional and named arguments behave the same way: they are the same until overwritten. Let me explain this with an example:

// my-component.js
export default Component.extend({}).reopenClass({
   positionalParams: ['a', 'b', 'c']
});

If we create this contextual component (component "my-component" 1 2 3 english="Good morning" galego="Bos días" spanish="Buenos días") and render it without any other properties, the component would get:

{
  a: 1,
  b: 2,
  c: 3,
  english: "Good morning",
  galego: "Bos días",
  spanish: "Buenos días"
}

If we were to overwrite any of the named properties, the values would be overwritten by these new values ((component (component "my-component" english="Hello") english="Good morning") would get only "Good morning" as the english value).

Given that positional parameters are but named parameters in disguise, I think they should behave the same: overwriting when possible.

Of course, it can be argued that positional parameters should behave like bind arguments but IMHO this would make components harder to use:

• Adding a positional parameter in a contextual component might make a different call error.
• You cannot be sure of being assigning the first positional parameter to a contextual component anymore.

And this led me to like this PR but maybe for the wrong reasons:

• If you cannot use positional parameters reliably, you will end up not using them except when really needed.
• If people cannot be sure of assigning the n-th positional parameter, APIs will need to be usable without positional parameters.

For the implementation and the tests, they LGTM.

[rwjblue]

The problem I see with concatenating positional parameters is that they do not really exist, they are but a trick.

I think this is only true for non-rest style positional params, once you specify positionalParams: 'stringHere' the changes in this PR actually opens quite a few more doors to you (e.g. "pseudo splatting" of positional params).

Furthermore, assigning a property via a positional parameter and a named argument at the same time would throw an error.

This is still true (I believe I added a few more tests around the condition).

I think current implementation has a simpler mental model since it made both positional and named arguments behave the same way: they are the same until overwritten.

I think this is actually an implementation detail that may change. As you know, in Glimmer 2 the argument munging that is done to support positional params is done at the component manager level which we are actively pushing to expose (in a somewhat limited way) via public API. Once that happens, its quite possible that component managers could be implemented in user space that expose positional params directly to the underlying components.

it can be argued that positional parameters should behave like bind arguments

I think that thinking of (component in relation to .bind is generally most folks mental model. We often tell folks to think of invoking a component as "invoking a function" (it is possibly the most common teaching analogy for components that I have seen...), in that mental model the simplest reasonable explanation of (component is via Function.prototype.bind...

And this led me to like this PR but maybe for the wrong reasons:

• If you cannot use positional parameters reliably, you will end up not using them except when really needed.
• If people cannot be sure of assigning the n-th positional parameter, APIs will need to be usable without positional parameters.

FWIW, I do not think these are the wrong reasons at all. I think its fairly clear that most component API's should be designed to use hash argument primarily for a number of reasons (e.g. it is unlikely that angle bracket components will ever support positional params).

[Serabe]

Let's merge this then :)

[chancancode]

We reviewed this and have one request: instead of allowing positional and named arguments to freely intermingle across layers, we should combine positional and named arguments separately and apply the mapping and assertion at the end.

[rwjblue]

Thanks for reviewing @chancancode / @wycats! I'll make those changes and let y'all know when its ready for another look...

[rwjblue]

FWIW, this change landed as part of #15828.