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
Closed

Conversation

@machty
Copy link
Contributor

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 machty mentioned this pull request Dec 10, 2015
@machty machty force-pushed the machty:splat-es6-2 branch from 2b7b0c7 to 35f48f4 Dec 11, 2015
@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,

This comment has been minimized.

Copy link
@kpdecker

kpdecker Dec 12, 2015

Collaborator

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)


shouldThrow(function() {
astFor('{{foo ...=lol ...baz}}');
}, Error, /Parse error on line 1/);

This comment has been minimized.

Copy link
@kpdecker

kpdecker Dec 12, 2015

Collaborator

Is {{foo ...=lol baz}} invalid as well?

This comment has been minimized.

Copy link
@machty

machty Dec 12, 2015

Author Contributor

Yes, I'll add a test. Basically this PR doesn't change the rule that params are on the left, hashy things on the right.

@@ -85,6 +85,23 @@ export function template(templateSpec, env) {
return typeof current === 'function' ? current.call(context) : current;
},

splat: function() {
return Utils.extend.apply(null, arguments);

This comment has been minimized.

Copy link
@kpdecker

kpdecker Dec 12, 2015

Collaborator

What is the first argument passed here? Are we ok with it being modified in all cases?

This comment has been minimized.

Copy link
@kpdecker

kpdecker Dec 12, 2015

Collaborator

Just saw this so please disregard :) Do we have test coverage asserting that the first object is not mutated?

this.hash = this.hashes.pop();
let hashPieces = this.hashPieces;
this.hashPieces = this.hashes.pop();
this.hashPiece = this.hashPieces && this.hashPieces[this.hashPieces.length - 1];

This comment has been minimized.

Copy link
@kpdecker

kpdecker Dec 12, 2015

Collaborator

What is this bit of code doing here?

This comment has been minimized.

Copy link
@machty

machty Dec 12, 2015

Author Contributor

What use to just be stack of hash objects is now a stack of hash piece arrays, and this.hashPiece refers to the last (current) hashPiece, so we restore it to that last element unless we've popped off all the hash piece arrays.

This comment has been minimized.

Copy link
@kpdecker

kpdecker Dec 12, 2015

Collaborator

Ok, maybe we can put a comment in the code to explain the restore operation?

]);
},

helperFunctionCall: function(helperName, helperOptions, splatMap) {
if (splatMap) {
let splatMapObj = {};

This comment has been minimized.

Copy link
@kpdecker

kpdecker Dec 12, 2015

Collaborator

Any thoughts on performance/code size of this vs. array with missing values notation? I.e. [,,1] vs. {"2":1}.

This comment has been minimized.

Copy link
@machty

machty Sep 30, 2017

Author Contributor

I don't know, don't have the perf wizard knowledge myself.

}

let argsWithSplatMap = helperOptions.params.slice();
argsWithSplatMap.push(this.objectLiteral(splatMapObj));

This comment has been minimized.

Copy link
@kpdecker

kpdecker Dec 12, 2015

Collaborator

nit: Can we do a concat here to avoid the intermediate array from the slice operation?

This comment has been minimized.

Copy link
@jamesarosen

jamesarosen Dec 17, 2016

push mutates. concat returns a new array. Either way the goal seems to be to create a new array to avoid mutating helperOptions.params. Though I suppose VMs might optimize a.concat(b) in ways they can't a.slice().push(b).

},

splatArgs: function() {
let splatMap = arguments[arguments.length - 1];

This comment has been minimized.

Copy link
@kpdecker

kpdecker Dec 12, 2015

Collaborator

nit: Would the code be clearer here if we did splatArgs(args, splatMap) {} rather than trying to extract from the end of the args list?
Alt: Lets pass splatMap as the first parameter so we can avoid the .length - 1 gymnastics.

@kpdecker
Copy link
Collaborator

kpdecker commented Dec 12, 2015

@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

kpdecker commented Feb 24, 2016

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

joeyjiron06 commented Feb 26, 2016

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

@rtablada
Copy link

rtablada commented Mar 23, 2016

@machty I want Spready

@morhook
Copy link

morhook commented Apr 18, 2016

I like this!!

@foxnewsnetwork
Copy link

foxnewsnetwork commented May 8, 2016

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

workmanw commented May 12, 2016

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

knownasilya commented Nov 21, 2016

What's the status on this?

@sglanzer
Copy link

sglanzer commented Nov 21, 2016

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.

andrew8er added a commit to anfema/website that referenced this pull request Sep 26, 2017
Slide components must take a model object as a "data" attribute, since splat operator is not merged yet: handlebars-lang/handlebars.js#1149

Re: 30
andrew8er added a commit to anfema/website that referenced this pull request Sep 26, 2017
Slide components must take a model object as a "data" attribute, since splat operator is not merged yet: handlebars-lang/handlebars.js#1149

Re: 30
andrew8er added a commit to anfema/website that referenced this pull request Sep 26, 2017
Slide components must take a model object as a "data" attribute, since splat operator is not merged yet: handlebars-lang/handlebars.js#1149

Re: 30
andrew8er added a commit to anfema/website that referenced this pull request Sep 27, 2017
Slide components must take a model object as a "data" attribute, since splat operator is not merged yet: handlebars-lang/handlebars.js#1149

Re: 30
andrew8er added a commit to anfema/website that referenced this pull request Sep 27, 2017
Slide components must take a model object as a "data" attribute, since splat operator is not merged yet: handlebars-lang/handlebars.js#1149

Re: 30
andrew8er added a commit to anfema/website that referenced this pull request Sep 28, 2017
Slide components must take a model object as a "data" attribute, since splat operator is not merged yet: handlebars-lang/handlebars.js#1149

Re: 30
andrew8er added a commit to anfema/website that referenced this pull request Sep 28, 2017
Slide components must take a model object as a "data" attribute, since splat operator is not merged yet: handlebars-lang/handlebars.js#1149

Re: 30
andrew8er added a commit to anfema/website that referenced this pull request Sep 29, 2017
Slide components must take a model object as a "data" attribute, since splat operator is not merged yet: handlebars-lang/handlebars.js#1149

Re: 30
andrew8er added a commit to anfema/website that referenced this pull request Sep 29, 2017
Slide components must take a model object as a "data" attribute, since splat operator is not merged yet: handlebars-lang/handlebars.js#1149

Re: 30
andrew8er added a commit to anfema/website that referenced this pull request Sep 29, 2017
Slide components must take a model object as a "data" attribute, since splat operator is not merged yet: handlebars-lang/handlebars.js#1149

Re: 30
@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 machty force-pushed the machty:splat-es6-2 branch from 35f48f4 to f855260 Sep 30, 2017
@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: #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.
andrew8er added a commit to anfema/website that referenced this pull request Oct 2, 2017
Slide components must take a model object as a "data" attribute, since splat operator is not merged yet: handlebars-lang/handlebars.js#1149

Re: 30
andrew8er added a commit to anfema/website that referenced this pull request Oct 5, 2017
Slide components must take a model object as a "data" attribute, since splat operator is not merged yet: handlebars-lang/handlebars.js#1149

Re: 30
@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

lsg-braymon commented Oct 8, 2018

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.

@nknapp nknapp closed this Nov 28, 2019
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

You can’t perform that action at this time.