Spread operator

[fdecampredon]

Shoul support correctly :

• [1, 2, 3, ...[a, b]]
• myFunc(a, b, ...[c, d]);
• myObject.myMethod(a, b, ...[c, d]);
• new MyClass(...[a, b])

Esprima parser does not support [1,2, ...[a,b], c, d] yet however the code has been created with that in mind and should support spread element as non final element of array/arguments out of the box when esprima will support it.

[subtleGradient]

I haven't looked at the diff yet, but… YES!!!1!
I've been wanting this. Thanks so much for hacking it up!

[fdecampredon]

np, I did it mainly because I want it too =)

[benjamn]

Can you add a comment with a few examples of how the spread operator transform works, like the one above for rest parameters?

[benjamn]

This PR might be a good opportunity to start thinking about supporting transforms that have runtime dependencies (cc @jeffmo), so that we don't have to generate so much boilerplate.

[fdecampredon]

yes I wanted to use directly Array.isArray(array) ? array: !function () { throw new TypeError() }() but that was not good since 'array' could be computed (and so it would have execute the computation 2 times) so I had to enclose it with closure, and did not change the code. I do that right now.

[benjamn]

Technically you need to examine the result of ClassIdent.apply(...) in case it returns an object, and (if so) replace resultIdent with that new value. I realize this is kind of pedantic, but I mention it as another argument in favor of handling logic like this inside some kind of runtime abstraction, rather than generating code that checks this case every time a ... operator is used.

[benjamn]

Backing up a step or two, I would prefer to have a discussion about the way the transform will work before a pull request is submitted, in a separate issue. Then, once we agree on the best way to simulate the ES6 feature with ES5 code, @fdecampredon (or whoever is interested) can implement that.

Tests passing isn't enough, because we're going to end up with a lot of interacting transforms, and we need to be absolutely confident about the design of each one by itself, if we're going to have any hope of combining them safely.

[fdecampredon]

Sorry, I just had the urge to play a little with it and started coding, then just asked in IRC if I should pull request or publish it in another repo. I should have submit an issue first.

[jeffmo]

Yea, runtime libs are pretty compelling from a highlevel perspective, but they do present a bit of complexity for the purposes of this project. It means the transforms this project ships with have to take a stance on a module system and/or other environmental aspects (like "make sure this thing is available before other things are" if we try to not take a stance on a module system).

It's at least nice to be able to say something as simple as "just run this on your code before you run your code". For now I think we should consider sticking with the boilerplate until we can at least come up with a consistent story around how we want to do the runtime libs.

I will take a look at this in more depth tomorrow and get back to you on any feedback. We'll want to be very sure that we're as close to spec compliant as we can be here and that this interacts nicely with other transforms -- so we're going to have to be a bit meticulous here. I'm looking forward to diving in!

[jeffmo]

Ok I was just thinking about the runtime library idea and looking a bit at how regenerator does it's thing. Maybe we can get away with a second (optional) transform that just inlines the runtime source at the top of the file.

This way the story around a runtime lib stays simple ("use the inlining transform for quick projects, but get the runtime in the environment on your own accord for prod/if you want to share across files").

Maybe this transform could be a good starting point for moving some of the more complex transforms in the same direction (I'm looking at you, class transform)

[fdecampredon]

Could be also cool to have regenerator integrated with jstransform

[benjamn]

I mean, I'm biased, but 👍 to that!

[fdecampredon]

Modified the code according to your feedback
@benjamn does my comments seems good to you ? I did not want to put in that comment the output result since it's huge and not really clear to read, but more the equivalence.
@subtleGradient I externalized the templates as much as possible like you suggested, however I don't know if the code is so much clear.

I discovered an edge case that I cannot implements :

function MyClass(a, b) { 
  return undefined;
}
var result = new MyClass(...[1, 2]); // result will be  { _proto: MyClass.prototype }

I don't think it's possible to emulate, but if you have an idea ....

[jeffmo]

Instead of a templates file, let's give the runtime lib a try and see where this takes us.

I'm envisioning something like the following (super rough sketch of what's in my head at least):

es6-spread-operator-visitors.js

var RUNTIME = '____JSTRANSFORM_SPREAD_RUNTIME____';

function visitProgram(traverse, node, path, state) {
  if (state.g.opts.includeSpreadRuntime) {
    utils.append(fs.readFileSync('./es6-spread-operator-runtime.js'), state);
  }
}
visitProgram.test = function(node) {
  return node.type === Syntax.Program;
};

function visitArrayExpressionWithSpread(traverse, node, path, state) {

  // I didn't test this exact logic, just winging it for example's sake

  utils.append('Array.prototype.concat.apply([],[', state);
  node.elements.forEach(function(element) {
    if (element.type === Syntax.SpreadElement) {
      utils.move(element.range[0] + 3, state); // jump past '...'
      utils.append('],', state);
      utils.append(RUNTIME + '.assertSpreadElement(', state);
      utils.catchup(element.range[1], state);
      utils.append('),[', state);
    } else {
      utils.move(element.range[0], state);
      utils.catchup(element.range[1], state);
    }
  });
  utils.catchup(node.range[1], state);
}

// ...etc...

es6-spread-operator-runtime.js

(function(global) {
  function assertSpreadElement(element) {
    if (!Array.isArray(element)) {
      throw new TypeError('Attempted to spread a non-array!');
    }
    return element;
  }

  global.____JSTRANSFORM_SPREAD_RUNTIME____ = {
    assertSpreadElement: assertSpreadElement
  };
})(this);

[fdecampredon]

sounds pretty good, especially since it could helps with handling 'iterable objects' instead of just array, I'll try to works on that tomorrow.

[jeffmo]

Ok, so I apologize for the wait, but I'm currently pretty heads down on getting our test runner to a state where it's ready to be published. Once we have a test runner that's published and usable, getting these pull requests pushed through should be much quicker since it'll be more of a matter of reviewing tests + test results!

That said, I don't think I'll actually have a chance to look at this thoroughly until sometime next week after all (I'll be out on vacation this weekend). I really appreciate your patience and effort on this :)

[fdecampredon]

No problem. , I'm actually trying your runtime proposition should I do it in a different branch, so spread operator could be included without runtime ? or should I directly try this on this PR ?

[jeffmo]

Doing a PR against master like you have here is fine

[fdecampredon]

@jeffmo I extracted some parts of the code into a runtime in the way described in your comment.
However I've not extracted part that would not have made the output smaller.

I also detected a problem in the transformation:
[[1, 2], ...[3, 4]] was transformed into [1, 2, 3, 4] I also fixed this problem.
Waiting for more comments

[sophiebits]

Note that #67 overlaps some with this and was merged yesterday.

[ghost]

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.