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

Splat redux with es6-ish syntax #1149

Closed
wants to merge 2 commits into from

Conversation

machty
Copy link
Contributor

@machty machty commented Dec 10, 2015

This PR includes and improves upon the first splat PR (#1128) based on discussions within #1050. Specifically, this PR provides (or will provide):

  • ES6-ish ...-based splatting syntax
  • splatting for hashes
  • splatting for positional params
  • splatting subexpressions e.g. ...(wat)

@machty
Copy link
Contributor Author

machty commented Dec 11, 2015

@kpdecker @nathanhammond @BenjaminHorn this is ready for review now; just got splatting positional params and subexpressions to work.

Notes:

  • I haven't figured out how to de-dupe the new grammar; I tried SPLAT? but it yielded grammar conflicts, even though to my eyes that's functionally equivalent to the grammar I explicitly specify
  • positional param splatting works as follows: if there's no splats, there's no change to how helpers are called today; if there's at least one splat, instead doing a .call on the helper, we first invoke a new runtime function splatArgs and then .apply those to the helper function.
  • I didn't implement splatting the various helper method missing methods; I'm not sure it's worth the bloat?

let params = this.setupFullMustacheParams(decorator, program, undefined),
let setup = this.setupFullMustacheParams(decorator, program, undefined),
params = setup.params,
splatMap = setup.splatMap,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Should we use destructuring here? (I forget what exact rules Handlebars has enabled, it was one of the earlier ES6 projects for me so it is likely a bit more conservative than need be)

@kpdecker
Copy link
Collaborator

@machty Generally this looks really good, had a few minor comments. Responding to your notes:

  1. I've seen similar issues when trying to do the same. Sometimes Jison gives ambiguous productions for things that to me seem like they should be unambiguous and are the same as the duplicated listing. Since it's in the compiler I'm not too worried about it not being completely DRY.
  2. Perfect
  3. For blockHelperMissing, that code is only run if the template has no args or hash, so you are correct that no changes are needed.

@kpdecker
Copy link
Collaborator

Pong?

@machty
Copy link
Contributor Author

machty commented Feb 24, 2016

We've been holding off on pulling the trigger on this while we flesh out some remaining semantics on the Ember side of things, particularly related to how splatting will work for so-called angle-bracket components.

@joeyjiron06
Copy link

when do you think this will get in? im desperately waiting for this :)

@rtablada
Copy link

@machty I want Spready

@morhook
Copy link

morhook commented Apr 18, 2016

I like this!!

@foxnewsnetwork
Copy link

While we're waiting spread and splat to come out in 5.0.0 handlebars, I wrote up a small Ember addon that handles spreading for Ember's component helper here: https://github.com/foxnewsnetwork/ember-component-helpers

That can be used by users of Ember2.0 and above like this:

Ember.Controller.extend({
  arrayArgs: ["admin.posts.post", 33, {class: "mdl-navigation__link", tagName: "li"}]
});
{{#component-apply "link-to" arrayArgs}}
  Your Link Text
{{/component-apply}}

You know, in case anyone else is particularly hungry for the spread after 1 year of waiting for it to come out like 2 straight months of looking at @rtablada 's animated gif of butter being spread on bread

@workmanw
Copy link

Just curious if there is any update on this. We could badly use this in our app. :)

@vomchik
Copy link

vomchik commented Jun 2, 2016

+1

@knownasilya
Copy link

What's the status on this?

@sglanzer-deprecated
Copy link

For anyone looking for another option while waiting https://github.com/ciena-blueplanet/ember-spread is mixin based, but works with dynamic hashes

@knownasilya
Copy link

knownasilya commented Nov 21, 2016

On another note, splats might work better as Element Modifiers (emberjs/rfcs#112), e.g. {{my-comp (splat params)}} or <my-comp {{splat params}}/>.

The downside is that element modifiers look like positional params, as @dfreeman mentioned on Slack.

@pjcarly
Copy link

pjcarly commented May 15, 2017

I'd use it exactly as you describe @machty

I currently have situations with a wrapper component like this, but then with 5 or 6 attributes that are being passed.

index/template.hbs
{{wrapper-component arg1=arg1 arg2=arg2 arg3=arg3}}

wrapper-component/template.hbs
{{wrapped-component arg1=arg1 arg2=arg2 arg3=arg3}}

it would be nice to not have to duplicate the passing of arguments in the wrapper-component's template

@workmanw
Copy link

👍 on a hash of options (named args). Probably wouldn't use the array much.

@foxnewsnetwork
Copy link

@machty From my personal experience, this is mostly an issue when trying to build a component to wrap {{link-to}} components (as oppose to extending LinkComponent). For example, if I were to create a {{my-fancy-link-to}} component that looks something like:

app/components/templates/my-fancy-link-to.hbs

{{yield}}
{{#link-to ...args }}
  <div class='fancy-stuff'>...</div>
{{/link-to}}
<div class='other-links'>
  {{#link-to "somewhere.else"}}
    ...
  {{/link-to}}
</div>

I would need to "splat" the ...args to properly emulate the functionality, but at the same time, I have no easy way of doing the splat

@gnapse
Copy link

gnapse commented May 15, 2017

I too have always looked for something like this for spreading a hash of options. And I also wanted to say that it's not just to avoid duplicating the enumeration of options in the component and the wrapper. It's also to make the wrapper component agnostic of the component it is wrapping. I've had use cases with components that could conceivably wrap totally different inner components, with largely different sets of possible options. The wrapper component need not know about these kind of stuff.

@XaserAcheron
Copy link

Confession time: when I was first learning Ember, I resorted to some shameful hacking to get around the exact case of having to pass a zillion variables through a wrapper component. I eventually gave in and started doing things the proper way, but being able to splat the hash (eww) would really save both my past and present self a few brain cells.

@LevelbossMike
Copy link

I'd also like to see this for the use-case of wrapper components. Right now I'm using ember-spread to do this:

http://blog.firstiwaslike.com/clean-ember-addon-component-customization-with-ember-spread/

but it would be great if that was built-in functionality.

@jamesarosen
Copy link

jamesarosen commented Jun 5, 2017

I have another potential use-case for this. I have a pagination-controls component that builds up a list of pages, then does

{{#each pages as |page|}}
  {{#link-to (query-params page=page.number)}}page.number{{/link-to}}
{{/each}}

I'd like to make the name of the query-param configurable:

{{my-pagination param='foosPage'}}

I can think of two ways of doing that, both of which require new syntax

// interpolate hash key:
{{#link-to (query-params [param]=page.number)}}

// hash-splat:
{{#link-to (query-params ...page.queryParamsHash)}}

(Because of how helper:query-params works, I might just be able to build up a { isQueryParams: true, values: { foosPage: 4 } } object and omit the (query-params ...) invocation altogether, but that's just because the implementation of that helper is so simple.)

@knownasilya
Copy link

Also the array splat is pretty important for positional params, {{#link-to dynamicRouteName ...routeParams}}. Currently, you cannot use link-to with dynamic params.

@gabrielgrant
Copy link

Is this only going to work for components, or will this also work for passing params to helpers? In practice, helpers seem to take positional params far more frequently than components do

@ldong
Copy link

ldong commented Jul 25, 2017

Cannot wait to see this get merged.

@stevenvachon
Copy link

I thought it was called "spread" in ES2015.

@jbescoyez
Copy link

jbescoyez commented Sep 30, 2017

@machty Thanks for this awesome PR. I'm wondering how comes this has not been merged yet.

3 questions:

  • Is there some blocker at the moment?
  • What workarounds are you using for handling dynamic options passing between components?
  • How can I help to see this merged?

@machty
Copy link
Contributor Author

machty commented Sep 30, 2017

Just pushed a rebase to fix merge conflicts.

@jbescoyez

  1. Basically the issue is that a large enough threshold of people are wanting this for Ember, but even if we land this syntax, the implementation for template data-binding on the Ember/Glimmer side of things still needs to happen, and particularly with Glimmer there are performance constraints that need to be considered before we ship a syntax that everyone might like but is unfortunately doomed to be untenably slow.
  2. See these comments from this thread: Splat redux with es6-ish syntax #1149 (comment) and https://github.com/ciena-blueplanet/ember-spread
  3. I'm not sure, it's really mostly blocked on Glimmer, which is developing quite rapidly (though admittedly this feature's been on the backburner). I've pinged the Ember/Glimmer team as to how to proceed with this PR. I'm thinking though that it probably makes sense for there to be an official Ember/Glimmer RFC with some backing from the core team, like we did with Named Blocks.

@nknapp
Copy link
Collaborator

nknapp commented Oct 7, 2017

Basically @wycats has to say that it should get merged. I promised only to fix bug and not add new features, when I asked for push access.

He wants a spec first, have a look at #1277

@Windvis
Copy link

Windvis commented Jul 23, 2018

Are there any plans on merging this (or implementing something similar)? Any news?

@lsg-braymon
Copy link

Any updates or news on this PR? Thanks!

@nknapp
Copy link
Collaborator

nknapp commented Oct 9, 2019

There is no update and for the last 2 years, no one has made a serious attempt to write a spec. I'm thinking about it from time to time, but it's a lot of work, I have different obligations, and for me it would be just for the fun of it. I am not using Handlebars in production anywhere and my primary concern is in fixing bugs, answering issues and merging and releasing minor improvements.

I have actually started an attempt here but it has been unchanged for a long time.

I doubt that anything will change here unless somebody (other than me) gets invested in this an starts writing.

@nknapp nknapp modified the milestones: 5.0.0, Backlog Oct 27, 2019
@nknapp
Copy link
Collaborator

nknapp commented Nov 28, 2019

Hi @machty. This PR is now open for 4 years and I feel pity that you have put ao much work in a PR that has then been ignored for so long. I haven't really looked at it until now because it was before my time, and @wycats asked me to only fix bugs while this is clearly a feature. That hasn't changed in principal and after talking to @wycats I think that development will go into another direction.

But I can ask @wycats to have a look and give his approval for merging this. I will do this only if you say you still need it.

The reason why I'm asking this is: I want to change some tooling and use "prettier" as a code formatter. This will make development much easier in the future, but it will make merging this request much more difficult.

I could just discard it, but I don't want to do that without asking first.

@knownasilya
Copy link

knownasilya commented Nov 28, 2019

I'd love to see this make it in.

@machty
Copy link
Contributor Author

machty commented Nov 28, 2019

I'm not going to be bothered at all if this ends up getting closed. Even if it does, it'll still leave a blueprint for what needs to happen to get this over the finish line.

I don't have the bandwidth to be a champion of this feature, but I believe one of the main blockers was that it wasn't clear whether implementing a "splat/spread" operator was going to be an easily-optimizable feature in Glimmer. But regardless of efficiency, it seems like an obvious win for component composition (and proven by React and others) to be able to forward all the args to a child component without enumerating all of them.

So I'd say close it for now so you can evolve the codebase and start using Prettier, but I wouldn't expect the debate to stop.

@nknapp
Copy link
Collaborator

nknapp commented Nov 28, 2019

I have some concerns that templates using this feature will become more difficult to understand, when trying to track the current evaluation context.

I'll gladly close it for now. I just didn't want to upset you.

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

Successfully merging this pull request may close these issues.

None yet