rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

Spreadable Arguments

Open Alonski opened this issue 4 years ago • 38 comments

Rendered

Alonski avatar Feb 12 '20 13:02 Alonski

Discussion in fork PR: https://github.com/Alonski/rfcs/pull/1

Alonski avatar Feb 12 '20 13:02 Alonski

This is awesome. What about abstracting a standalone ...@? I'm new to some of this, but it would be very cool to be able to do something like:

<!-- HBS-->
<MyButton ...@buttonArgs/>
// component.js
get buttonArgs(){
    return {color: "blue"}
}

In that case ...@arguments is the equivalent of ...@args

averydev avatar Feb 14 '20 01:02 averydev

@averydev Though it is a cool idea, what about there is an argument named @buttonArgs as well?

The reason we use @ or this. in .hbs files is we want to avoid ambiguous references, so it should be like ...this.buttonArgs in your case if it becomes a real feature.

nightire avatar Feb 14 '20 04:02 nightire

@nightire good point. The @ does signify that it's being an argument as opposed to the local context, so my example isn't consistent with that.

I only just learned about the ...attributes feature, which is fantastic, and in my understanding relates specifically to the attributes that are applied to the element form of the component:

<!-- Parent HBS-->
<MyButton class="sandwich" @buttonText="Hi!"/>
<!-- My Button Component HBS-->
<button ...attributes>{{@buttonText}}</button>
<!-- Resuting HTML-->
<button class="sandwich">Hi!</button>

Mulling on it a bit more,...attributes is special purpose, and if you {{log attributes}} you get undefined (for good reason). My thought is that it should remain an outlier and continue to stand out, rather than expanding the use of stand-alone ....

How about this: {{...someObject}}

<!-- A local object-->
<MyButton ...attributes {{...this.buttonArgs}}/>

<!-- A proxy, most likely just component arguments-->
<MyButton ...attributes {{...this.args}}/>

<!-- An object / hash passed in as an argument-->
<MyButton ...attributes {{...@someHash}}/>

That makes the whole thing pretty simple to fit into the current mental model. There's just a {{...}} helper that splats an object or proxy into @arguments on a component.

I'm not sure what would happen if the target isn't a component... either it would splat key/values onto the element as attributes onto the element, or ignore them entirely. V0 probably just ignore them for simplicity sake:

Option 1:

<!-- Template-->
<button {{...this.buttonArgs}}>Hi!</button>

//JS:
get buttonArgs(){
return {
  title:"ohhai"
}

<!-- Result -->
<button title="ohhai">Hi!</hi>

Option 2

<!-- Template-->
<button {{...this.buttonArgs}}>Hi!</button>

<!--Result -->
<button>Hi!</hi>

averydev avatar Feb 15 '20 00:02 averydev

@averydev I had a similar idea to creating a custom helper such as {{...this.someObj}} but @pzuraq explained to me via DM that this would be a custom helper and as such makes less sense. In the Alternatives section I mention the possibility of arbitrary spreads of attributes and arguments like this:

<SubComponent ...{{this.someSpecificAttrs}} ...@{{this.someSpecificArgs}}></SubComponent>

Alonski avatar Feb 15 '20 12:02 Alonski

I'm not convinced by the ...@arguments syntax to be honest. I fear just doing the same as for attributes is not a good fit for many cases.

If the wrapper component has arguments itself, only a subset of all arguments should be passed through to the wrapped component.

But this wouldn't be supported by the proposed syntax. It would be an all or nothing choice.

I fear many developers would pass all arguments through including the ones of the wrapper component. That might work as long as these arguments aren't used by the wrapped component. But it could break at any point of time as adding new arguments wouldn't require a major version bump.

I think a syntax that covers most use cases must support at least exclusion of some arguments. But such a syntax might get so complex that directly adding support for arbitrary arguments might be more valuable as that one supports even more use cases (e.g. dynamic component invocation with different arguments depending on the component).

It's another story for ... attributes. The component does not have access to these attributes. So there can't be any that are local to the component and shouldn't be passed through.

TL;DR: The proposed syntax would only support a few use cases. Adding support for arbitrary splat arguments directly should be the focus in my opinion.

jelhan avatar Feb 16 '20 23:02 jelhan

@jelhan that’s a very valid concern, and actually exactly why this proposal is what it is. This is already a common issue with ...attributes for instance.

The issue is that creating a fully general spread syntax, that doesn’t just apply to this special case, opens up a large can of worms. What about spreading within helpers and modifiers? What about spreading objects that aren’t ...attributes or ...@arguments? What about spreading collections other than objects (e.g. arrays, sets, maps). Should you be able to destructure as well as spread? Etc.

The goal was to keep the scope of this particular proposal small, without limiting future possibilities, so that we could iterate and continue to add features just like the ones you’ve outlined. I personally would like to see a very generalized spread syntax, as well as the ability to be more precise when spreading and applying attributes, but I think this would make sense as a first step in that direction, and would help solve some immediate needs now.

pzuraq avatar Feb 16 '20 23:02 pzuraq

I agree that we should try to solve easy cases first. But I fear that the proposed feature would only support very few use cases. It's easy to assume that it would support more use cases than it can. I think this paragraph in the RFC is misleading:

A possible component in the wild that could benfit from this could be ember-power-select. This component has to explicitely pass on many arguments to its child components. With "splarguments" half or more of this template could be cut down.

This is only true if the child component is provided by the same addon. That's the only scenario in which passing through more arguments than needed is safe. Otherwise it's a footgun cause the child component could add an additional argument that clashes with arguments of the wrapper component in any minor release.

I think this limitation should at least be mentioned in the RFC. And the Ember Power Select example should be updated to avoid wrong assumptions about covered use cases.

I fear an example of such a wrong assumption with ember-bootstrap-power-select by @simonihmig. I agree that it would profit a lot from being able to pass through arguments but I don't think the proposed ...@arguments feature could be used for it. The component provided by that addon passes most of it's arguments through to Ember Power Select but not all. The wrapper component has a optionLabelPath argument of it's own. Passing that one also would break as soon as Ember Power Select adds an argument with that name itself.

jelhan avatar Feb 17 '20 13:02 jelhan

Generally in favor of some way to "passthrough" args, but in order to determine what ...@arguments does, we need to first define @arguments and its relationship to this.args in glimmer component classes. E.g. if the constructor decides to mutate something inside, does @arguments get the same thing? Like @jelhan says, I think the use case is different than ...attributes since arguments are much more flexible and used for many more things.

mehulkar avatar Feb 17 '20 22:02 mehulkar

@mehulkar this.args is immutable, so I don't think we need to worry about issues of syncing state between the two. I do think a slightly more formal definition would be nice, and I think it would be something along the lines of:

@arguments is a keyword representing all of the arguments passed to the component. It directly references the arguments' original values, so the values cannot be changed or modified via the component manager or component class.

Eventually, we could add a non-keyword way to spread specific arguments (and attributes).

pzuraq avatar Feb 17 '20 23:02 pzuraq

I'm thinking about a "picking" ability, something like:

{{! forwarding all passing attributes }}
<div ...attributes>
  <input>
</div> 

{{! forwarding some passing attributes }}
<div ...attributes('class' 'data-theme')>
  <input ...attributes('id' 'name' 'type' 'value')>
</div>

and it can be applied to @arguments if it's a thing, make sense?

nightire avatar Feb 18 '20 02:02 nightire

I'm thinking about a "picking" ability

The main use case of ... attributes is to support HTML attributes you aren't aware of at implementation time. Something similar is needed for arguments l. Therefore it's more about exclusion of some than inclusion of a known list in my opinion.

Not sure but exclusion of some attributes might be already supported with current syntax by setting these attributes after ... attributes to their default value. Or even undefined? 🤔

But something similar wouldn't work for ...@arguments cause they would still be present on this.args of a glimmer component. And the component may work differently if an argument is passed - even if it's value is undefinded.

jelhan avatar Feb 18 '20 10:02 jelhan

FWIW, I think that this would make a lot of sense without a special syntax, if we gave you the ability to spread arbitrary objects:

class MyComponent extends Component {
  get restArgs() {
    let { foo, bar, ...rest } = this.args;

    return rest;
  }
}
<MySubComponent ...@{{this.restArgs}} />

For attributes, we would need to make them accessible to JS somehow I think. But like I said before, I think this could come later, potentially. It could also be added to this RFC, but that may make it take longer to get consensus on and to implement.

pzuraq avatar Feb 18 '20 20:02 pzuraq

My thoughts on this.

Disclaimer: I read your comments AFTER I had my thoughts sorted, so mine weren't influenced by yours but I will make connections to what you wrote.

First of all I separated the problem into a spreadable syntax and then how to combine it with arguments.

Spreadable Syntax

The idea here is that we have a KVO and when spreaded into something, it will receive the keys with their values as individual arguments. What could it look like:

class DummyComponent extends Component {
	properties = {
		foo: 'bar',
		baz: 'bam'
	};
}
{{! dummy-component/template.hbs }}
<MyComponent @params={{...this.properties}}>

and here is the Args of MyComponent:

interface MyComponentArgs {
	params: {
		foo: string;
		baz: string;
	};
}

which will receive it that way. That would then allow us to use helpers for modifying the arguments being spread in:

<MyComponent @params={{...(filter this.properties 'foo'}}>
<MyComponent @params={{...(assign this.properties this.moreProperties}}>

(I moved @pzuraq example code from TS to hbs)

I didn't thought about all the technical challenges here. Luckily we have a @pzuraq for this:

What about spreading objects that aren’t ...attributes or ...@arguments? What about spreading collections other than objects (e.g. arrays, sets, maps). Should you be able to destructure as well as spread? Etc.

Simply spoken: the spreadable syntax must make sure to pass on a KVO that keeps things such as tracking intact.

Spreadable Arguments

This needs two things: A reserved name and a special slot to pass them down. I think we can all easily agree on using @arguments as the reserved name, which then would open up two possible ways to pass them down:

<MyComponent {{...@arguments}}>
<MyComponent @arguments={{...@arguments}}>

Arguments are available in the backing up component as this.args but not available in the template with a reserved word. The question here is do we need a reserved word? Can we do this instead:

<MyComponent {{...this.args}}>

Technically this would work I guess ;) but would disconnect the idea of arguments being prefixed with @-sigils and make it harder to visually identify them in the template. Basically we would have something like this:

Template Code
@arguments this.args

-> shouldn't they be named the same then?

Serving Multiple Problems

By accident I'd say this would allow to address the problems @jelhan was describing:

I'm not convinced by the ...@arguments syntax to be honest. I fear just doing the same as for attributes is not a good fit for many cases.

Now that would be possible:

<MyComponent {{...(filter @arguments 'foo' 'bar')}} @subsetForSpecialUseCase={{...(assign (filter @arguments 'baz') this.props)}}>

which can be consumed as such:

{{! my-component/template.hbs }}
<MyHeavyComponentBuilder {{...@arguments}} as |builder|>
	<builder.Content {{...@subsetForSpecialUseCase}}>
		{{yield}}
	</builder.Content>
</MyHeavyComponentBuilder>

Synchronosity between @arguments and attributes

As attributes and arguments might work similarly here (on a syntax level) - it might be helpful to think about one pattern of syntax that serves both parameters and would only be need to learned once instead of two different patterns an ember developer has to learn for things that look very similar.

As much as I want this, I can't at the moment see a way to make this happen. Yet I want to make everybody aware that ...attributes and ...@arguments shouldn't be handled much differently from a consumer perspective.

Moving forward

I think it is super fine to keep the scope for the first implementation small and provide solutions for immediate problems. However, the whole context need to be thought through to the end at first and then sliced into implementation chunks.

As much as I want this, I don't want it to be a blocker for ourselves moving forward in half a year or year.

gossi avatar Feb 22 '20 10:02 gossi

{{! dummy-component/template.hbs }}
<MyComponent @params={{...this.properties}}>

This seems off to me. Let's think about how this would translate into JS:

let args = {
  params: ...this.properties
};

This isn't valid syntax. The issue is we need to spread into something.

let args = {
  params: { ...this.properties }
};

The natural analog in HBS would be something like:

<MyComponent @params={{hash ...this.properties}}>

The spreadable syntax must make sure to pass on a KVO that keeps things such as tracking intact.

I think that this would be really unfortunate. It would be very convenient to be able to spread an array of arguments into a helper.

{{foo ...this.params}}

Or, thinking about the opposite. Using rest syntax within a block:

<MyComponent as |...params|>
  {{foo ...params}}
</MyComponent>

This would absolutely be valuable. I think the key thing to figure out is for a handlebars expression, how do we tell if a value is meant to be spread as an object or an array. In JS, we know by the context of the spread:

foo(...arr); // spreads like array
let bar = [...arr]; // spreads like array
let baz = { ...arr }; // spreads like object, even though it's an array

Unfortunately we don't have a way to distinguish with mustache statements.

<!-- foo can receive both named and position args, so ...params could be targeting either -->
{{foo ...params}}

I was hoping to punt on this. I think one option would be to allow @ syntax specifically for this within a helper:

<!-- ...params spreads as an array, ...@params spreads as an object -->
{{foo ...params ...@params}} 

But this seems bad to me, since @ isn't used for anything else in that context. Open to other suggestions here as well.

I think it is super fine to keep the scope for the first implementation small and provide solutions for immediate problems. However, the whole context need to be thought through to the end at first and then sliced into implementation chunks.

Agreed here, in that I think we need to make sure we aren't designing a syntax that would prevent future possibilities. I don't think we need to have the whole plan nailed down to land this first though.

pzuraq avatar Feb 22 '20 16:02 pzuraq

I think the key thing to figure out is for a handlebars expression, how do we tell if a value is meant to be spread as an object or an array.

I was only thinking about the case for them to be passed as KVO/object - I think I made it myself to easy because:

I think that this would be really unfortunate. It would be very convenient to be able to spread an array of arguments into a helper.

I love to extend my thinking on that. Thank you for all that examples you put to it. But as you also said (regarding {{foo ...params ...@params}}):

But this seems bad to me, since @ isn't used for anything else in that context. Open to other suggestions here as well.

That's what I avoided with the limited scope of only treating hashes. The obvious parts then would be:

<MyComponent {{hash (assign ...@arguments (foo (array ...this.props)))}}

I don't like it ! Putting regular javascript around:

<MyComponent {{hash (assign { ...@arguments } (foo [ ...this.props ]))}}

or in simplified version for spreadable args only:

<MyComponent {{hash { ...@arguments }}}

I'd consider it the verbose way that needs syntactic sugar now - would you agree?

gossi avatar Feb 22 '20 21:02 gossi

That's what I avoided with the limited scope of only treating hashes.

I think if we’re going to discuss the scope of hashes in general, we must also discuss the scope of arrays and other collections.

This is why I favored the original design. If ...@arguments is effectively a keyword, then it wouldn’t block any of the other possibilities. It makes no claims on the behavior of spread syntax within handlebars expressions, only within angle-brackets, and in that case there is only one possibility.

I’ll think on syntax for handlebars statements more though. It would be nice for us to have a vision for that.

pzuraq avatar Feb 22 '20 21:02 pzuraq

We might want to consider ...@arguments={{this.myArgs}}. ...@arguments could be a shorthand for ...@arguments={{this.args}}.

It would support helpers (...@arguments={{hash foo=bar}}) and would also work for attributes: ...attributes={{this.myAttrs}}.

Maybe it should be ...@args={{this.myArgs}} cause having ...@arguments but this.args might be confusing.

jelhan avatar Feb 22 '20 21:02 jelhan

Another possibility would be to use literal syntax. We've been discussing it for a while potentially, something along the lines of:

{{hello (a=b)}}
{{hello [a.b c "hello"]}}

<!-- equivalent to -->
{{hello (hash a=b)}}
{{hello (array a.b c "hello"))}}

In this world, we could say that array spread is ...identifier and object-spread is ...(identifier):

<!-- Applies the values of obj to the named arguments of hello -->
{{hello ...(obj)}}

<!-- Applies the values of arr to the positional arguments of hello -->
{{hello ...arr}}

<!-- Spreads `obj` into a new object, adding/overriding some values, and then passes it as the first parameter to yield -->
{{yield (...(obj) foo=this.bar)}} 

I think the one case that I can see where this would get verbose is if you wanted to create a new object and spread it:

{{hello ...((foo=this.bar))}}

But I think the JS equivalent here is also pretty confusing to read:

hello({ ...{ bar: 'baz' } });

And also, I imagine, pretty uncommon.

pzuraq avatar Feb 23 '20 18:02 pzuraq

@pzuraq This would be used with @ and this?

{{hello ...(this.obj)}}?

Alonski avatar Feb 23 '20 20:02 Alonski

@Alonski I was only discussing spread syntax within handlebars statements at that point. Expanding it to angle bracket syntax, I think there are two options. We can continue to use @ to distinguish between args and attrs within handlebars statements:

<Foo ...@arguments ...attributes />

<Foo ...@{{this.dynamicArgs}} ...{{this.dynamicAttrs}} />

I think this is fine-ish. The other option that I can think of would be to have ... within handlebars statements spread arguments, and ... externally spread attrs:

<Foo {{...(arguments)}} ...attributes />

<Foo {{...(this.dynamicArgs)}} ...{{this.dynamicAttrs}} />

This case also has an edge case that doesn't make sense, and would probably trip people up:

<Foo {{...arr}} />

We can't spread an array to a component invoked with angle brackets, so this would have to error. It might be tempting to say that for this special case, we can omit the top level () or any other special syntax to denote object-spread, but I think we should likely be consistent wherever spread syntax is used within handlebars, to prevent confusion.

pzuraq avatar Feb 23 '20 20:02 pzuraq

Maybe related. Storybook announced an RFC to spread args into components: https://docs.google.com/document/d/1Mhp1UFRCKCsN8pjlfPdz8ZdisgjNXeMXpXvGoALjxYM

gossi avatar Mar 16 '20 09:03 gossi

Late to the party here... we are dying on this one right now in a complex enterprise build that uses a dynamic select-type component we built (like power-select but oriented to our domain) but wraps it for very specific cases in about a dozen places - every time we improve/alter the base to answer a change request from the customer we end up doing a lot of touch up. This would be a great feature. @pzuraq - perhaps another syntax option to avoid the collision you mention in your most recent comment (put the @ inside - i.e. contemplate it as a binding process on the input rather than a binding process on the target):

<Foo {{@...someDynamicArgs}} />

This kind of ember work is still above my pay grade or I'd pile in.

DLiblik avatar May 24 '21 15:05 DLiblik

I came across a significant piece of additional motivation for this RFC this week and wanted to write it down before I forgot. There is actually a significant gap in what you can do by manually forwarding arguments. Namely: you cannot forward a non-defined argument as non-defined (as opposed to explicitly defining an argument with a value of undefined). This is primarily an interop hazard when wrapping Classic components, so the motivation will decrease over time, but it's visible in a variety of ways at runtime regardless.

Consider this example Classic component as a simple way to motivate the example:

// example.js
import Component from '@ember/component';

export default Component.extend({
  value: 123,
});
{{! example.hbs }}
<p>The value is: {{this.value}}</p>

Because of how arguments are set on the class, overriding the default value from the prototype, users could simply invoke example without an argument and get the default value, because nothing was overriding it.[^good] So this invocation—

<Example />

—would render this HTML:

<p>The value is: 123</p>

Explicitly passing a value argument would override it. So this invocation—

<Example @value={{456}} />

—would render this HTML:

<p>The value is: 456</p>

...including if you passed undefined explicitly:

<Example @value={{undefined}} />
<p>The value is: </p>

This last bit is the key here, because if we want to wrap a Classic component—say, just hypothetically 😭 because you needed to A/B test the rollout of the conversion of important component in your app to Octane—then you would need to do something like this:

{{#if useNew}}
  <ExampleOctane @value={{@value}} />
{{else}}
  <Example @value={{@value}} />
{{/if}}

This works fine when the caller does pass a value. But when the caller does not pass a value, the result is that we pass <Example @value={{@value}} /> and the {{@value}} is undefined—but we pass it in explicitly, so we are in the third example above.

This all makes perfect sense if we think in terms of JS. Object spread by definition does not include any keys which are not defined on the source objects (how could it?):

const args = {};
const result = { value: 123, ...args }; // { value: 123 };
const args = { value: 456 };
const result = { value: 123, ...args }; // { value: 456 };
const args = { value: undefined };
const result = { value: 123, ...args }; // { value: undefined };

You can work around this, using the {{component}} helper, but it is quite annoying: it means that this concern leaks out of wherever you are doing this forwarding and becomes something callers have to care about.

example of using component

We can create a helper which returns the right class based on a condition. (This example assumes the first-class component feature from Ember 3.25 and the service import from Ember 4.1; you can do the same thing with string names earlier than that.)

import Helper from '@ember/component/helper';
import { service } from '@ember/service';
import ExampleOctane from '../components/example-octane';
import Example from '../components/example';

export default class FeatureTestedExample extends Helper {
  @service featureFlag;

  compute() {
    return this.featureFlags.isEnabled('my-new-thing')
      ? ExampleOctane
      : Example;
  }
}
{{#let (component (feature-tested-example)) as |Example|}}
  <Example />
{{/let}}

(In some cases, you could also do this by yielding a component, but in the case where I hit this, that would have required updating dozens of call sites in a fairly risky way.) Although this workaround gets the job done, it is suggestive of the challenges here, and it is also not intuitive or obvious. And while the problems here are most obvious for Classic components (indeed, this kind of thing was one of several major motivations for putting args in their own object instead of directly onto the backing class), it would similarly be a problem for anything which did actually depend on the difference between an unset field and a *field with value of undefined. ...@arguments would solve that neatly.

[^good]: Whether this exact approach was a good pattern or not is a separate issue; in practice this was very common and indeed widely recommended.

chriskrycho avatar Mar 10 '22 00:03 chriskrycho

I would like spreadable arguments to avoid having to do this:

{{! simplified example! }}
{{#if @model}}
  <LinkTo @route={{@route}} @model={{@model}}>{{yield}}</LinkTo>
{{else if @models}}
  <LinkTo @route={{@route}} @models={{@models}}>{{yield}}</LinkTo>
{{else}}
  <LinkTo @route={{@route}}>{{yield}}</LinkTo>
{{/if}}

One would expect to be able to write:

<LinkTo
  @route={{@route}}
  @model={{@model}}
  @models={{@models}}
>
  {{yield}}
</LinkTo>

And would prefer to write:

<LinkTo ...@arguments>{{yield}}</LinkTo>

But like @chriskrycho says, you can't forward a non defined argument. So this particular example results in You cannot provide both the '@model' and '@models'

amk221 avatar Mar 10 '22 00:03 amk221

Is there still interest in moving this forward?

wagenet avatar Jul 24 '22 00:07 wagenet

yes. I think it's spread is a suuuuuper nice feature for any language to have in 2020+

NullVoxPopuli avatar Jul 24 '22 00:07 NullVoxPopuli

I've had this on my todo list to work on it for almost 2 years 🙃

Alonski avatar Jul 24 '22 09:07 Alonski

Agreed. Would love to have this feature! I echo @amk221's use case.

chbonser avatar Jul 25 '22 13:07 chbonser

Indeed, it would be amazing to have this!


@amk221 indeed LinkTo is a place where I missed this a few times. While the solution is a getter like this:

get models() {
  return this.args.model ? [this.args.model] : this.args.models ?? [];
}

and just pass @models={{this.models}}.

this is really not as elegant as <LinkTo ...@arguments>. Especially when new arguments are added to <LinkTo, this would still work, which IMHO is much more important, allowing me to write more future-proof wrapper-components.

luxzeitlos avatar Aug 14 '22 03:08 luxzeitlos