bublejs / buble

https://buble.surge.sh
MIT License
871 stars 67 forks source link

Using spread on an object method generates breaking syntax #87

Open fskreuz opened 6 years ago

fskreuz commented 6 years ago

Here's a minimal representation of the original code, which works well on the browser:

const foo = { bar: [] }
const baz = true ? [1,2,3] : []
foo.bar.push(...baz)

// [1,2,3]
console.log(foo.bar)

When transpiled, the entire push operation appears to be ignored.

var foo = { bar: [] }
var baz = true ? [1,2,3] : []
(ref = foo.bar).push.apply(ref, [1,2,3])

// []
console.log(foo.bar)
var ref;

The current workaround is to add ; at the end of the ternary, breaking off the second and third line:

const foo = { bar: [] }
const baz = true ? [1,2,3] : [];
foo.bar.push(...baz)

// [1,2,3]
console.log(foo.bar)

Buble should automatically add the ; because it added the parens. The consumer should not be aware about adding ; due to implementation, especially when the code works without it before transpilation.

adrianheine commented 6 years ago

That's right, the emitted code should work regardless of the used semicolon style in the transpiled code.

nathancahill commented 6 years ago

I feel like changing the code generation (ref... to ;(ref... would be the safest fix here. Anywhere we're adding ( should guard against this.

nathancahill commented 6 years ago

Reading the source more, I think an acceptable fix would be prepending the created declaration var ref; to the line before (ref = foo.bar). Right now, ref is pulled to the top of the block with introStatementGenerators. In practice, it won't make a difference since var is hoisted anyway.

This would output:

var foo = { bar: [] }
var baz = true ? [1,2,3] : []
var ref;
(ref = foo.bar).push.apply(ref, [1,2,3])
amsik commented 6 years ago

Probably it is not a good way: var ref;. It helps in most cases, but I founded a bug:

class MySuperArray extends Array {
  constructor(someProp, items) {
    super();

    this.someProp = someProp
    this.push(...items)
  }
}

This code transpiles to:

...
    this.someProp = someProp /* still we have problem here */
    (ref = this).push.apply(ref, items)
...

Maybe need to add a semicolon before (ref = this)? We used to do this a long time ago using IIEF

    this.someProp = someProp
    ;(ref = this).push.apply(ref, items)
huanghuiquan commented 6 years ago

Any plan to fix the issue?

adrianheine commented 6 years ago

@nathancahill started to fix this in #95.