Splat redux with es6-ish syntax #1149

Open
wants to merge 2 commits into
from

Projects

None yet
@machty
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 referenced this pull request Dec 10, 2015
Closed

Splat operator #1050

@machty
Contributor
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?
@kpdecker kpdecker commented on the diff Dec 12, 2015
lib/handlebars/compiler/compiler.js
@@ -156,11 +156,13 @@ Compiler.prototype = {
DecoratorBlock(decorator) {
let program = decorator.program && this.compileProgram(decorator.program);
- let params = this.setupFullMustacheParams(decorator, program, undefined),
+ let setup = this.setupFullMustacheParams(decorator, program, undefined),
+ params = setup.params,
+ splatMap = setup.splatMap,
@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)

@kpdecker kpdecker commented on the diff Dec 12, 2015
spec/parser.js
@@ -215,6 +222,14 @@ describe('parser', function() {
shouldThrow(function() {
astFor('{{{{goodbyes}}}} {{{{/hellos}}}}');
}, Error, /goodbyes doesn't match hellos/);
+
+ shouldThrow(function() {
+ astFor('{{foo ...}}');
+ }, Error, /Parse error on line 1/);
+
+ shouldThrow(function() {
+ astFor('{{foo ...=lol ...baz}}');
+ }, Error, /Parse error on line 1/);
@kpdecker
kpdecker Dec 12, 2015 Collaborator

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

@machty
machty Dec 12, 2015 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.

@kpdecker kpdecker commented on the diff Dec 12, 2015
lib/handlebars/runtime.js
@@ -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);
@kpdecker
kpdecker Dec 12, 2015 Collaborator

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

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

@kpdecker kpdecker commented on the diff Dec 12, 2015
lib/handlebars/compiler/javascript-compiler.js
},
popHash: function() {
- let hash = this.hash;
- this.hash = this.hashes.pop();
+ let hashPieces = this.hashPieces;
+ this.hashPieces = this.hashes.pop();
+ this.hashPiece = this.hashPieces && this.hashPieces[this.hashPieces.length - 1];
@kpdecker
kpdecker Dec 12, 2015 Collaborator

What is this bit of code doing here?

@machty
machty Dec 12, 2015 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.

@kpdecker
kpdecker Dec 12, 2015 Collaborator

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

@kpdecker kpdecker commented on the diff Dec 12, 2015
lib/handlebars/compiler/javascript-compiler.js
]);
},
+ helperFunctionCall: function(helperName, helperOptions, splatMap) {
+ if (splatMap) {
+ let splatMapObj = {};
@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}.

@kpdecker kpdecker commented on the diff Dec 12, 2015
lib/handlebars/compiler/javascript-compiler.js
]);
},
+ helperFunctionCall: function(helperName, helperOptions, splatMap) {
+ if (splatMap) {
+ let splatMapObj = {};
+ for (let i = 0, l = splatMap.length; i < l; ++i) {
+ splatMapObj[splatMap[i]] = 1;
+ }
+
+ let argsWithSplatMap = helperOptions.params.slice();
+ argsWithSplatMap.push(this.objectLiteral(splatMapObj));
@kpdecker
kpdecker Dec 12, 2015 Collaborator

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

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

@kpdecker kpdecker commented on the diff Dec 12, 2015
lib/handlebars/runtime.js
@@ -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);
+ },
+
+ splatArgs: function() {
+ let splatMap = arguments[arguments.length - 1];
@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
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 kpdecker added the feature label Dec 12, 2015
@kpdecker kpdecker added this to the 5.0.0 milestone Dec 12, 2015
@kpdecker
Collaborator

Pong?

@machty
Contributor
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

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

@rtablada

@machty I want Spready

@morhook
morhook commented Apr 18, 2016

I like this!!

@foxnewsnetwork

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

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

@vomchik
vomchik commented Jun 2, 2016

+1

@nathanpalmer nathanpalmer referenced this pull request in unchartedcode/semantic-ui-ember-modal Jun 8, 2016
Merged

Instead of depending on render we depend on a dynamic component #1

@knownasilya

What's the status on this?

@sglanzer

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 edited

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.

@jamesarosen jamesarosen added a commit to jamesarosen/ember-i18n that referenced this pull request Dec 20, 2016
@k-fish @jamesarosen k-fish + jamesarosen t helper supports context object
Previously, the `{{t}}` helper, like the `i18n.t` utility, accepted a
translation key and a context hash. This worked when the Handlebars template
had the individual keys and values (or value bindings) for the context,
but didn't when there was a pre-built object that represented the context.

Now it accepts a second ordered (non-hash) argument that represents the
context as an object. Hash context properties override those from
the context object.

```hbs
{{t 'some.key' someObject prop=value}}
```

is approximately the same as

```js
i18n.t('some.key', Object.assign({}, someObject, { prop: value }))
```

This is a workaround for the fact that Handlebars does not yet have a syntax
for splatting an object into hash arguments.

See wycats/handlebars.js#1050
See wycats/handlebars.js#1128
See wycats/handlebars.js#1149
Closes #423
0854a3f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment