Skip to content

Splat redux with es6-ish syntax - #1149

Closed
machty wants to merge 2 commits into
handlebars-lang:masterfrom
machty:splat-es6-2
Closed

machty wants to merge 2 commits into
handlebars-lang:masterfrom
machty:splat-es6-2

Conversation

@machty

@machty machty commented Dec 10, 2015

Copy link
Copy Markdown
Contributor

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

machty commented Dec 11, 2015

Copy link
Copy Markdown
Contributor Author

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

Comment thread lib/handlebars/compiler/compiler.js Outdated

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

Pong?

@machty

machty commented Feb 24, 2016

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown

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

@rtablada

Copy link
Copy Markdown

@machty I want Spready

@morhook

morhook commented Apr 18, 2016

Copy link
Copy Markdown

I like this!!

@foxnewsnetwork

Copy link
Copy Markdown

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
Copy Markdown

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

@vomchik

vomchik commented Jun 2, 2016

Copy link
Copy Markdown

+1

@knownasilya

Copy link
Copy Markdown

What's the status on this?

@sglanzer-fire

Copy link
Copy Markdown

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

@knownasilya

knownasilya commented Nov 21, 2016 •

Copy link
Copy Markdown

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

pjcarly commented May 15, 2017

Copy link
Copy Markdown

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
Copy Markdown

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

@foxnewsnetwork

Copy link
Copy Markdown

@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

gnapse commented May 15, 2017 •

Copy link
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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

jamesarosen commented Jun 5, 2017 •

Copy link
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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

ldong commented Jul 25, 2017

Copy link
Copy Markdown

Cannot wait to see this get merged.

@stevenvachon

Copy link
Copy Markdown

I thought it was called "spread" in ES2015.

@jbescoyez

jbescoyez commented Sep 30, 2017 •

Copy link
Copy Markdown

@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

machty commented Sep 30, 2017 •

Copy link
Copy Markdown
Contributor Author

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

nknapp commented Oct 7, 2017

Copy link
Copy Markdown
Collaborator

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

Windvis commented Jul 23, 2018

Copy link
Copy Markdown

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

@lsg-braymon

Copy link
Copy Markdown

Any updates or news on this PR? Thanks!

@nknapp

nknapp commented Oct 9, 2019 •

Copy link
Copy Markdown
Collaborator

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

nknapp commented Nov 28, 2019

Copy link
Copy Markdown
Collaborator

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

knownasilya commented Nov 28, 2019 •

Copy link
Copy Markdown

I'd love to see this make it in.

@machty

machty commented Nov 28, 2019

Copy link
Copy Markdown
Contributor Author

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

nknapp commented Nov 28, 2019

Copy link
Copy Markdown
Collaborator

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.