bublejs / buble

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

Computed property following a spread that's nested under a computed is missing closing parens #139

Closed fskreuz closed 5 years ago

fskreuz commented 6 years ago

Object.assign will be missing parens if you assign an object to a computed that has a spread followed by a computed. Demo

Input:

var z1 = { [b]: { ...c, [d]: 'Hello, World!' } }

Output:

var obj;
var z1 = {};

// Actual
z1[b] = Object.assign({}, c, ( obj = {}, obj[d] = 'Hello, World!', obj )

// Expected
z1[b] = Object.assign({}, c, ( obj = {}, obj[d] = 'Hello, World!', obj ))

The bug happens regardless of depth. Demo

Input:

var z1 = { [b]: { ...c, [d]: { ...e, [f]: 'Hello, World!' } } }

Output:

var obj, obj$1;
var z1 = {};

// Actual
z1[b] = Object.assign({}, c, ( obj$1 = {}, obj$1[d] = Object.assign({}, e, ( obj = {}, obj[f] = 'Hello, World!', obj ), obj$1 )

// Expected
z1[b] = Object.assign({}, c, ( obj$1 = {}, obj$1[d] = Object.assign({}, e, ( obj = {}, obj[f] = 'Hello, World!', obj )), obj$1 ))

This bug is a blocker especially for those who use a Redux-like approach to recreate state (nested shallow copies).

fskreuz commented 6 years ago
const closing = code.slice(beginEnd, end);
code.prependLeft(moveStart, closing);
code.remove(beginEnd, end);

The actual culprit is https://github.com/Rich-Harris/buble/blob/master/src/program/types/ObjectExpression.js#L226 .

// Before this code runs
var z1 = {; z1[b] = Object.assign({}, c,( obj = {}, obj[d] = 'Hello, World!', obj ))}

// After this code runs
var z1 = {)}; z1[b] = Object.assign({}, c,( obj = {}, obj[d] = 'Hello, World!', obj )

The code above tries to move the closing } from the tip of the original z1 to close the newly appended var z1 = {. But when does that, it moves the } and the character before it. The missing parens was collateral damage. What's weird is that beginEnd was 44 and end was 45 - you'd expect slice to return one character, but it returns two.

adrianheine commented 5 years ago

Would you consider submitting a pull request for this?

fskreuz commented 5 years ago

I think I tried to fix this before, and I discovered that this was caused by MagicString (which I have not wrapped my head around just yet). It's most likely due to the order of string manipulation/what MagicString thinks was on the original string at some index. I remember that I couldn't reproduce the issue in small scale.

adrianheine commented 5 years ago

I think I fixed this, but it's pretty arcane.