Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spreadable Arguments #593

Open
wants to merge 10 commits into
base: master
from

Conversation

@Alonski
Copy link
Member

Alonski commented Feb 12, 2020

Rendered

Alonski and others added 8 commits Jun 26, 2018
Updated regarding pzuraqs' comments.
Co-Authored-By: Chris Garrett <me@pzuraq.com>
@Alonski

This comment has been minimized.

Copy link
Member Author

Alonski commented Feb 12, 2020

Discussion in fork PR:
Alonski#1

<!-- Using the same rule on arbitrary spreads though, we no longer have context -->
<SubComponent ...{{this.someSpecificAttrs}} ...{{this.someSpecificArgs}}></SubCompnoent>
Comment on lines 93 to 94

This comment has been minimized.

Copy link
@buschtoens

buschtoens Feb 12, 2020

Arbitrary spreads would be an awesome addition! I'm unsure, whether they might explode the scope of this RFC, but I would still love to have them eventually!


A possible component in the wild that could benfit from this could be
[ember-power-select](https://github.com/cibernox/ember-power-select/blob/master/addon/templates/components/power-select.hbs).
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 comment has been minimized.

Copy link
@simonihmig

simonihmig Feb 13, 2020

Contributor

In case someone needs further proof how useful this could be, I'll gladly show you this beauty:
https://github.com/kaliber5/ember-bootstrap-power-select/blob/master/addon/templates/components/bs-form/element/control/power-select.hbs
😱🤯🤪

<SubComponent ...{{this.someSpecificAttrs}} ...{{this.someSpecificArgs}}></SubCompnoent>
<!-- Maybe prepending `@`? -->
<SubComponent ...attributes ...@arguments></SubComponent>

This comment has been minimized.

Copy link
@BobrImperator

BobrImperator Feb 13, 2020

I think having an ...@arguments would be amazing, it greatly corresponds to what We have now

@amyrlam amyrlam mentioned this pull request Feb 13, 2020
10 of 19 tasks complete
@averydev

This comment has been minimized.

Copy link

averydev commented Feb 14, 2020

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

@nightire

This comment has been minimized.

Copy link

nightire commented Feb 14, 2020

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

@averydev

This comment has been minimized.

Copy link

averydev commented Feb 15, 2020

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

This comment has been minimized.

Copy link
Member Author

Alonski commented Feb 15, 2020

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

This comment has been minimized.

Copy link

jelhan commented Feb 16, 2020

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.

@pzuraq

This comment has been minimized.

Copy link
Contributor

pzuraq commented Feb 16, 2020

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

@jelhan

This comment has been minimized.

Copy link

jelhan commented Feb 17, 2020

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.

@mehulkar

This comment has been minimized.

Copy link

mehulkar commented Feb 17, 2020

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.

@pzuraq

This comment has been minimized.

Copy link
Contributor

pzuraq commented Feb 17, 2020

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

@nightire

This comment has been minimized.

Copy link

nightire commented Feb 18, 2020

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?

@jelhan

This comment has been minimized.

Copy link

jelhan commented Feb 18, 2020

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.

@pzuraq

This comment has been minimized.

Copy link
Contributor

pzuraq commented Feb 18, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.