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

transform-es2015-modules-commonjs destructuring error #5768

Closed
jonhartnett opened this issue May 24, 2017 · 6 comments · Fixed by #5811
Closed

transform-es2015-modules-commonjs destructuring error #5768

jonhartnett opened this issue May 24, 2017 · 6 comments · Fixed by #5811
Labels
Has PR i: bug outdated A closed issue/PR that is archived due to age. Recommended to make a new issue

Comments

@jonhartnett
Copy link

When destructuring into an exported variable, transform-es2015-modules-commonjs fails to generate code to keep the local variable and the exports value in sync.

Input Code

export let x = 0;
export function f1(){
    x = 1;
}
export function f2(){
    [x] = [2];
}
export function f3(){
    ({x} = {x: 3});
}

Babel Configuration (.babelrc, package.json, cli command)

{
  "plugins": ["transform-es2015-modules-commonjs"]
}

Expected Generated Code

"use strict";

Object.defineProperty(exports, "__esModule", {
    value: true
});
exports.f1 = f1;
exports.f2 = f2;
exports.f3 = f3;
let x = exports.x = 0;
function f1() {
    exports.x = x = 1;
}
function f2() {
    [x] = [2];
    exports.x = x; //this is one possible solution
}
function f3() {
    ({ x } = { x: 3 });
    exports.x = x;
}

Current Generated Code

"use strict";

Object.defineProperty(exports, "__esModule", {
    value: true
});
exports.f1 = f1;
exports.f2 = f2;
exports.f3 = f3;
let x = exports.x = 0;
function f1() {
    exports.x = x = 1;
}
function f2() {
    [x] = [2];
}
function f3() {
    ({ x } = { x: 3 });
}

Possible Solution

Add a line to keep the local variable and the export value in sync when destructuring into export variables.

software version(s)
Babel 6.24.1
babel-plugin-transform-es2015-modules-commonjs 6.24.1
@babel-bot
Copy link
Collaborator

Hey @jonhartnett! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

@joshwnj
Copy link
Contributor

joshwnj commented Jun 1, 2017

@jonhartnett this is interesting. Do you know whether this is even expected behaviour of javascript?

I'm not 100% up to scratch with how the bindings work, but it may be because in this case x is value rather than a reference.

For example, if we're dealing with an object reference instead of a value:

let y = exports.y = { val: 1 };

function f2 () {
  [y.val] = [2]
}

calling f2() does update both y and exports.y.

If anyone can weigh in on whether this is expected behaviour according to the module spec, that would be very helpful :)

@jonhartnett
Copy link
Author

As far as I know module bindings should be "live", any updates should be visible from outside the module. Furthermore, the line in f3 should behave as nothing more than syntactic sugar for
x = ({x: 3}).x
You can see from the differing output if you make that change, that this is indeed a bug and not "unexpected" expected behavior.

joshwnj added a commit to joshwnj/babel that referenced this issue Jun 1, 2017
- adds a failing test based on description in babel#5768
- handles ObjectPattern and ArrayPattern
@joshwnj
Copy link
Contributor

joshwnj commented Jun 1, 2017

@jonhartnett ahh you're right - I was thinking from the point of view of the commonjs module but I see what you mean now.

I think I've got something working for this, just pushing a PR for review now.

@joshwnj joshwnj mentioned this issue Jun 1, 2017
@joshwnj
Copy link
Contributor

joshwnj commented Jun 1, 2017

@jonhartnett when you have a moment please try #5811 and see if that works for you.

@jonhartnett
Copy link
Author

@joshwnj That works perfectly

@hzoo hzoo added the i: bug label Jun 27, 2017
hzoo pushed a commit that referenced this issue Jun 27, 2017
* Fix destructured exports

- adds a failing test based on description in #5768
- handles ObjectPattern and ArrayPattern

* use export assignment template
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 4, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Has PR i: bug outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants