Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
fskreuz opened this issue Jun 5, 2018 · 4 comments
Labels

Comments

@fskreuz
Copy link
Contributor

fskreuz commented Jun 5, 2018

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 fskreuz changed the title Nested computed and object spread combo transpiles to code missing a closing parens Object.assign from nested object spread missing a closing parens Jun 18, 2018
@fskreuz fskreuz changed the title Object.assign from nested object spread missing a closing parens Object.assign from nested object spread followed by a computed property is missing a closing parens Jun 18, 2018
@fskreuz fskreuz changed the title Object.assign from nested object spread followed by a computed property is missing a closing parens Computed property following a spread that's nested under a computed does not add closing parens Jun 19, 2018
@fskreuz fskreuz changed the title Computed property following a spread that's nested under a computed does not add closing parens Computed property following a spread that's nested under a computed is missing closing parens Jun 19, 2018
@fskreuz
Copy link
Contributor Author

fskreuz commented Jun 20, 2018

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
Copy link
Member

Would you consider submitting a pull request for this?

@adrianheine adrianheine added the bug label Oct 7, 2018
@fskreuz
Copy link
Contributor Author

fskreuz commented Oct 9, 2018

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
Copy link
Member

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

adrianheine added a commit that referenced this issue Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants