Skip to content

Commit

Permalink
Parenthesize a first spread element
Browse files Browse the repository at this point in the history
In most cases this is unnecessary, but just occasionally it is
necessary, notably in ternaries; for `a ? b : c.concat(…)` is very
different from `(a ? b : c).concat(…)`.

We could be cleverer about whether parentheses are needed, but Bublé
doesn’t generally seem to go out of its way to *absolutely* minimise
such things, and I’m also not sure whether there’s a good way to do
that, or whether you have to start doing checks “is it a
ParenthesizedExpression, CallExpression, NewExpression, &c.”
With the opening parenthesis leaked into CallExpression.js, it’d become
messier, too.

I’m not clear why the insertion of the opening parenthesis had to happen
in src/program/types/CallExpression.js rather than src/utils/spread.js
when start === element.start; but if I did it inside spread(), if I used
appendLeft or prependLeft it’d end up like `.apply((Math, a).concat(…))`
instead of `.apply(Math, (a).concat(…))`, and if I used appendRight or
prependRight, nothing happened.

Fixes bublejs#177.
  • Loading branch information
chris-morgan committed Jan 10, 2019
1 parent 77cf57d commit 376006d
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 16 deletions.
6 changes: 4 additions & 2 deletions src/program/types/CallExpression.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,9 @@ export default class CallExpression extends Node {
_super.noCall = true; // bit hacky...

if (this.arguments.length > 1) {
if (firstArgument.type !== 'SpreadElement') {
if (firstArgument.type === 'SpreadElement') {
code.prependRight(firstArgument.start, `( `);
} else {
code.prependRight(firstArgument.start, `[ `);
}

Expand All @@ -85,7 +87,7 @@ export default class CallExpression extends Node {
code.prependRight(firstArgument.start, `${context}, `);
} else {
if (firstArgument.type === 'SpreadElement') {
code.appendLeft(firstArgument.start, `${context}, `);
code.appendLeft(firstArgument.start, `${context}, ( `);
} else {
code.appendLeft(firstArgument.start, `${context}, [ `);
}
Expand Down
9 changes: 7 additions & 2 deletions src/utils/spread.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,13 @@ export default function spread(
const previousElement = elements[firstSpreadIndex - 1];

if (!previousElement) {
code.remove(start, element.start);
code.overwrite(element.end, elements[1].start, '.concat( ');
// We parenthesize it to handle ternaries like [...a ? b : c].
// (We could be smarter about it, but with the parenthesizing having
// spread to CallExpression.js, it sounds too much like hard work.)
if (start !== element.start) {
code.overwrite(start, element.start, '( ');
} // else CallExpression called us, and it puts the ( in itself.
code.overwrite(element.end, elements[1].start, ' ).concat( ');
} else {
code.overwrite(previousElement.end, element.start, ' ].concat( ');
}
Expand Down
2 changes: 1 addition & 1 deletion test/samples/classes-no-named-function-expressions.js
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ module.exports = [
var y = [], len = arguments.length - 1;
while ( len-- > 0 ) y[ len ] = arguments[ len + 1 ];
Bar.prototype.qux.apply(this, x.concat( y ));
Bar.prototype.qux.apply(this, ( x ).concat( y ));
};
Foo.prototype.fob = function ( x, y ) {
var this$1 = this;
Expand Down
2 changes: 1 addition & 1 deletion test/samples/classes.js
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ module.exports = [
var y = [], len = arguments.length - 1;
while ( len-- > 0 ) y[ len ] = arguments[ len + 1 ];
Bar.prototype.qux.apply(this, x.concat( y ));
Bar.prototype.qux.apply(this, ( x ).concat( y ));
};
Foo.prototype.fob = function fob ( x, y ) {
var this$1 = this;
Expand Down
28 changes: 18 additions & 10 deletions test/samples/spread-operator.js
Original file line number Diff line number Diff line change
Expand Up @@ -302,27 +302,27 @@ module.exports = [
{
description: 'transpiles multiple spread operators in an array',
input: `var arr = [ ...a, ...b, ...c ];`,
output: `var arr = a.concat( b, c );`
output: `var arr = ( a ).concat( b, c );`
},

{
description:
'transpiles multiple spread operators in an array with trailing comma',
input: `var arr = [ ...a, ...b, ...c, ];`,
output: `var arr = a.concat( b, c );`
output: `var arr = ( a ).concat( b, c );`
},

{
description: 'transpiles mixture of spread and non-spread elements',
input: `var arr = [ ...a, b, ...c, d ];`,
output: `var arr = a.concat( [b], c, [d] );`
output: `var arr = ( a ).concat( [b], c, [d] );`
},

{
description:
'transpiles mixture of spread and non-spread elements in array with trailing comma',
input: `var arr = [ ...a, b, ...c, d, ];`,
output: `var arr = a.concat( [b], c, [d] );`
output: `var arr = ( a ).concat( [b], c, [d] );`
},

{
Expand Down Expand Up @@ -385,14 +385,14 @@ module.exports = [
{
description: 'transpiles multiple spread operators in function call',
input: `var max = Math.max( ...theseValues, ...thoseValues );`,
output: `var max = Math.max.apply( Math, theseValues.concat( thoseValues ) );`
output: `var max = Math.max.apply( Math, ( theseValues ).concat( thoseValues ) );`
},

{
description:
'transpiles mixture of spread and non-spread operators in function call',
input: `var max = Math.max( ...a, b, ...c, d );`,
output: `var max = Math.max.apply( Math, a.concat( [b], c, [d] ) );`
output: `var max = Math.max.apply( Math, ( a ).concat( [b], c, [d] ) );`
},

{
Expand Down Expand Up @@ -597,9 +597,9 @@ module.exports = [
`,

output: `
f(b).concat( ["n"]);
f(b).concat( ['n']);
f(b).concat( ["n"]);
( f(b) ).concat( ["n"]);
( f(b) ).concat( ['n']);
( f(b) ).concat( ["n"]);
`
},

Expand All @@ -608,7 +608,7 @@ module.exports = [

input: `[...A, () => 'B']`,

output: `A.concat( [function () { return 'B'; }])`
output: `( A ).concat( [function () { return 'B'; }])`
},

{
Expand All @@ -617,5 +617,13 @@ module.exports = [
input: `new X(...A, () => 'B')`,

output: `new (Function.prototype.bind.apply( X, [ null ].concat( A, [function () { return 'B'; }]) ))`
},

{
description: 'transpiles a first spread element comprising a ternary operator',

input: '[...a ? b : c, d]',

output: '( a ? b : c ).concat( [d])',
}
];

0 comments on commit 376006d

Please sign in to comment.