Fix commonjs exports with destructuring. #5469

yavorsky · 2017-03-15T13:51:40Z

Q	A
Patch: Bug Fix?	y
Major: Breaking Change?	n
Minor: New Feature?	n
Deprecations?	n
Spec Compliancy?	y
Tests Added/Pass?	y
Fixed Tickets	Fixes babel/babel-preset-env#193
License	MIT
Doc PR
Dependency Changes

Original issue
While we are exporting VariableDeclaration with ObjectPattern inside using just transform-es2015-modules-commonjs we are loosing ExportsAssignment:
In

export const { baz } = {};

Out

const { baz } = {};

Expected

const { baz } = {};
exports.baz = baz;

It works as expected with transform-es2015-destructuring:
Out with transform-es2015-destructuring

var _ref = {};
const baz = _ref.baz;
exports.baz = baz;

Also 1 test was added.

mention-bot · 2017-03-15T13:51:41Z

@yavorsky, thanks for your PR! By analyzing the history of the files in this pull request, we identified @hzoo, @loganfsmyth and @benjamn to be potential reviewers.

babel-bot · 2017-03-15T13:58:11Z

Hey @yavorsky! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

codecov · 2017-03-15T14:08:39Z

Codecov Report

Merging #5469 into master will increase coverage by 0.02%.
The diff coverage is 91.3%.

@@            Coverage Diff             @@
##           master    #5469      +/-   ##
==========================================
+ Coverage   84.49%   84.52%   +0.02%     
==========================================
  Files         204      204              
  Lines        9597     9622      +25     
  Branches     2695     2703       +8     
==========================================
+ Hits         8109     8133      +24     
  Misses       1001     1001              
- Partials      487      488       +1

Impacted Files	Coverage Δ
...gin-transform-es2015-modules-commonjs/src/index.js	`94.6% <91.3%> (+0.11%)`	⬆️
packages/babel-traverse/src/visitors.js	`85.71% <0%> (-0.96%)`	⬇️
packages/babel-types/src/validators.js	`84.95% <0%> (+0.27%)`	⬆️
...bel-plugin-transform-es2015-classes/src/vanilla.js	`90.59% <0%> (+0.42%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71e4336...f2f226b. Read the comment docs.

aaronang · 2017-03-16T07:11:21Z

Looks good to me except that the test is failing.

danez · 2017-03-16T16:15:49Z

packages/babel-plugin-transform-es2015-modules-commonjs/src/index.js

+                    } else if (id.isObjectPattern()) {
+                      for (let i = 0; i < id.node.properties.length; i++) {
+                        const prop = id.node.properties[i];
+                        if (!t.isRestProperty(prop)) {


Shouldn't the rest property also be handled?

for example

export const {foo: test, ...tester} = {};

@danez Probably yes, but for now: 1) we haven't envs to use it without transform-object-rest-spread and 2) seems like we need babel-plugin-syntax-object-rest-spread for rest properties: repl

It's fine for people to have enabled support for the syntax without enabling the actual transformation, so I think it would be good to handle.

loganfsmyth · 2017-03-16T16:59:26Z

packages/babel-plugin-transform-es2015-modules-commonjs/src/index.js

@@ -306,6 +306,15 @@ export default function () {
                      addTo(exports, id.node.name, id.node);
                      init.replaceWith(buildExportsAssignment(id.node, init.node).expression);
                      nonHoistedExportNames[id.node.name] = true;
+                    } else if (id.isObjectPattern()) {


Should we be handling ArrayPattern too?

@loganfsmyth Sure! Going to add. Thanks!

Could the ArrayPattern be a separate PR?

@xtuc I've started work on it but could pick it out to a separate PR

babel-bot · 2017-03-18T14:32:11Z

Hey @yavorsky! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

babel-bot · 2017-03-18T22:39:49Z

Hey @yavorsky! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

xtuc · 2017-03-19T10:20:33Z

packages/babel-plugin-transform-es2015-modules-commonjs/src/index.js

@@ -129,6 +129,10 @@ export default function () {
  return {
    inherits: require("babel-plugin-transform-strict-mode"),

+    manipulateOptions(opts, parserOpts) {
+      parserOpts.plugins.push("objectRestSpread");
+    },


Would be better to inherits from babel-plugin-syntax-object-rest-spread. It seems multiple inheritance is not possible (at least didn't found an example).

To avoid that duplicated code, we could merge the config with the syntax plugin. What do you think?

@xtuc Yes! Unfortunately, I didn't find multiple inheritance too. Can you please point me if there util for merging configs?

It seems there are no merging util for configuration. I was thinking to use Object.assign.

Yes, just added Object.assign with babel-plugin-transform-object-assign as devDep to support node < 4.

babel-bot · 2017-03-19T12:08:22Z

Hey @yavorsky! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

Jessidhia · 2017-03-22T05:30:20Z

packages/babel-plugin-transform-es2015-modules-commonjs/src/index.js

-          path.replaceWith(t.identifier("undefined"));
-        }
-      },
+  return Object.assign(


This only works by coincidence because no keys clash, no actual merging is done.

Maybe it's ok in this case, but it's still... weird 😕

yeah, I agree. Do you know other ways to inherit from multiple plugins?

Not really 😢

I think it's worth to add this feature to core. Going to try today 😕

loganfsmyth · 2017-03-22T22:40:44Z

...s2015-modules-commonjs/test/fixtures/strict/export-const-destructuring-object-rest/actual.js

@@ -1 +0,0 @@
-export const { foo, ...bar } = {};


This test specifically seems like a reasonable thing to have, if you'd like to keep it as a devDep for the test.

idris · 2017-06-08T02:47:34Z

Ping! Looks like this is done. Can it be merged?

Fix commonjs exports with destructuring. by yavorsky · Pull Request #5469 · babel/babel babel/babel#5469

mpyw · 2017-06-21T13:25:31Z

I'm also waiting this PR merged...

moducks/moducks@0d3a92d

Fix commonjs exports with destructuring. by yavorsky · Pull Request #5469 · babel/babel babel/babel#5469

jridgewell · 2017-06-21T20:38:17Z

This will also need to be ported to 7.0.

yavorsky · 2017-06-21T20:39:27Z

@jridgewell yes! on it.

Yeti-or · 2017-07-24T23:56:00Z

Guys, will it be released under 6.x ?

yavorsky added 2 commits March 15, 2017 15:35

Fix exports while destructuring.

f81d749

Add test for exports destructuring.

9b410be

Remove exports definition.

0c2e3b1

existentialism added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Mar 16, 2017

aaronang approved these changes Mar 16, 2017

View reviewed changes

danez reviewed Mar 16, 2017

View reviewed changes

loganfsmyth reviewed Mar 16, 2017

View reviewed changes

yavorsky added 4 commits March 18, 2017 15:12

Add array pattern to exports destructuring.

b5bb89b

Consider default parameters for array pattern.

06f67e1

Consider rest params for array pattern in exports.

b608e28

Consider default params for object pattern.

45b4174

Fix object rest params for exports.

c82b084

Fix export array rest expected output.

18d6ba9

xtuc reviewed Mar 19, 2017

View reviewed changes

Add object rest spread syntax using plugin.

c42e027

Fix visitor inheritance.

7035401

Jessidhia reviewed Mar 22, 2017

View reviewed changes

yavorsky mentioned this pull request Mar 22, 2017

Multiple inheritances for plugins #5527

Closed

Remove object-rest-spread syntax.

ddfb6f2

loganfsmyth reviewed Mar 22, 2017

View reviewed changes

Add syntax-object-rest-spread as devDep for tests.

f2f226b

mpyw added a commit to moducks/moducks that referenced this pull request Jun 21, 2017

Needs es2015 until "export const { }" bug fixed

0d3a92d

Fix commonjs exports with destructuring. by yavorsky · Pull Request #5469 · babel/babel babel/babel#5469

ghost pushed a commit to moducks/moducks that referenced this pull request Jun 21, 2017

Needs es2015 until "export const { }" bug fixed

cbfb270

Fix commonjs exports with destructuring. by yavorsky · Pull Request #5469 · babel/babel babel/babel#5469

jridgewell approved these changes Jun 21, 2017

View reviewed changes

jridgewell merged commit 64eafad into babel:master Jun 21, 2017

yavorsky mentioned this pull request Jun 26, 2017

7.0 port: Fix commonjs exports with destructuring. #5886

Merged

mpyw added a commit to moducks/moducks that referenced this pull request Sep 4, 2017

Use env instead of es2015 ( babel/babel#5469 )

18c4d5b

mpyw mentioned this pull request Sep 12, 2017

Update dependencies (To fix Babel bug) facebook/create-react-app#3110

Closed

hzoo mentioned this pull request Oct 3, 2017

export const {variable} = otherVariable babel/babel-preset-env#193

Closed

snyk-bot mentioned this pull request Aug 30, 2019

[Snyk] Upgrade babel-runtime from 6.18.0 to 6.26.0 aarlaud/two-factor-loginattempt-issue-MCV#1

Open

snyk-bot mentioned this pull request Sep 19, 2019

[Snyk] Upgrade babel-plugin-transform-object-rest-spread from 6.23.0 to 6.26.0 ali8889/etherwallet#2

Open

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix commonjs exports with destructuring. #5469

Fix commonjs exports with destructuring. #5469

yavorsky commented Mar 15, 2017

mention-bot commented Mar 15, 2017

babel-bot commented Mar 15, 2017

codecov bot commented Mar 15, 2017 •

edited

aaronang commented Mar 16, 2017

danez Mar 16, 2017

yavorsky Mar 16, 2017

loganfsmyth Mar 16, 2017

loganfsmyth Mar 16, 2017

yavorsky Mar 16, 2017

xtuc Mar 17, 2017

yavorsky Mar 17, 2017

babel-bot commented Mar 18, 2017

babel-bot commented Mar 18, 2017

xtuc Mar 19, 2017

yavorsky Mar 19, 2017 •

edited

xtuc Mar 19, 2017

yavorsky Mar 19, 2017 •

edited

babel-bot commented Mar 19, 2017

Jessidhia Mar 22, 2017

yavorsky Mar 22, 2017

Jessidhia Mar 22, 2017

yavorsky Mar 22, 2017

loganfsmyth Mar 22, 2017

idris commented Jun 8, 2017

mpyw commented Jun 21, 2017 •

edited

jridgewell commented Jun 21, 2017

yavorsky commented Jun 21, 2017

Yeti-or commented Jul 24, 2017

Fix commonjs exports with destructuring. #5469

Fix commonjs exports with destructuring. #5469

Conversation

yavorsky commented Mar 15, 2017

mention-bot commented Mar 15, 2017

babel-bot commented Mar 15, 2017

codecov bot commented Mar 15, 2017 • edited

Codecov Report

aaronang commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

babel-bot commented Mar 18, 2017

babel-bot commented Mar 18, 2017

Choose a reason for hiding this comment

yavorsky Mar 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yavorsky Mar 19, 2017 • edited

Choose a reason for hiding this comment

babel-bot commented Mar 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idris commented Jun 8, 2017

mpyw commented Jun 21, 2017 • edited

jridgewell commented Jun 21, 2017

yavorsky commented Jun 21, 2017

Yeti-or commented Jul 24, 2017

codecov bot commented Mar 15, 2017 •

edited

yavorsky Mar 19, 2017 •

edited

yavorsky Mar 19, 2017 •

edited

mpyw commented Jun 21, 2017 •

edited