Skip to content

Fix 5768#5811

Merged
hzoo merged 2 commits intobabel:masterfrom
joshwnj:fix-5768
Jun 27, 2017
Merged

Fix 5768#5811
hzoo merged 2 commits intobabel:masterfrom
joshwnj:fix-5768

Conversation

@joshwnj
Copy link
Copy Markdown
Contributor

@joshwnj joshwnj commented Jun 1, 2017

Q A
Patch: Bug Fix? yes
Major: Breaking Change? no
Minor: New Feature? no
Deprecations? no
Spec Compliancy? yes
Tests Added/Pass? yes
Fixed Tickets Fixes #5768
License MIT
Doc PR no
Dependency Changes no

As described in #5768, the current behaviour treats exports correctly if they are assigned a value normally, but incorrectly if they are assigned a value via destructuring.

This PR adds a failing test based on description in #5768, and updates to make the test pass.

The isIdentifier case is unchanged, I've just added new cases for ObjectPattern and ArrayPattern. There might be a more elegant way to frame this, so please suggest if you see one :)

PS. first PR so please let me know if I missed any steps

joshwnj added 2 commits June 2, 2017 08:11
@joshwnj
Fix destructured exports
1172779 
- adds a failing test based on description in babel#5768
- handles ObjectPattern and ArrayPattern
@joshwnj
use export assignment template
123beb5
@mention-bot
Copy link
Copy Markdown

mention-bot commented Jun 1, 2017

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

@joshwnj joshwnj mentioned this pull request Jun 1, 2017
Closed
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 1, 2017 •
edited
Loading

Codecov Report

Merging #5811 into master will decrease coverage by <.01%.
The diff coverage is 86.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5811      +/-   ##
==========================================
- Coverage   84.75%   84.74%   -0.01%     
==========================================
  Files         204      204              
  Lines        9608     9622      +14     
  Branches     2702     2708       +6     
==========================================
+ Hits         8143     8154      +11     
  Misses        981      981              
- Partials      484      487       +3
Impacted Files Coverage Δ
...gin-transform-es2015-modules-commonjs/src/index.js 93.53% <86.95%> (-0.97%) ⬇️
...bel-plugin-transform-es2015-classes/src/vanilla.js 89.74% <0%> (-0.43%) ⬇️
packages/babel-traverse/src/path/context.js 85.34% <0%> (ø) ⬆️
packages/babel-traverse/src/visitors.js 86.66% <0%> (+0.95%) ⬆️

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 489cf90...123beb5. Read the comment docs.

@jridgewell jridgewell added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Jun 2, 2017
@hzoo hzoo requested a review from loganfsmyth June 9, 2017 15:14
@hzoo
Copy link
Copy Markdown
Member

hzoo commented Jun 27, 2017

Nice work @joshwnj!

@hzoo hzoo merged commit 3cf4cee into babel:master Jun 27, 2017
@hzoo
Copy link
Copy Markdown
Member

hzoo commented Jun 27, 2017

Hm realized this is also against master instead of 7.0 heh

@joshwnj
Copy link
Copy Markdown
Contributor Author

joshwnj commented Jun 27, 2017

@hzoo cheers!

Hm realized this is also against master instead of 7.0 heh

oh, sorry! :) So for future reference all PRs like this should target the 7.0 branch?

@joshwnj joshwnj deleted the fix-5768 branch June 27, 2017 22:05
@hzoo
Copy link
Copy Markdown
Member

hzoo commented Jun 27, 2017

Well master (6.x) should be 7.0 but we haven't switched it 😛

@joshwnj
Copy link
Copy Markdown
Contributor Author

joshwnj commented Jun 27, 2017

I see, no worries. Want me to put together another PR against 7.0 for this, or you've got it?

@jridgewell
Copy link
Copy Markdown
Member

jridgewell commented Jun 27, 2017

That'd be great if you did it!

@joshwnj
Copy link
Copy Markdown
Contributor Author

joshwnj commented Jun 27, 2017

Sure - I'll link it here when it's ready

@joshwnj joshwnj mentioned this pull request Jun 27, 2017
Merged
@joshwnj
Copy link
Copy Markdown
Contributor Author

joshwnj commented Jun 27, 2017

Ok build's passed so should be ready to go: #5891

This was referenced Mar 14, 2018
Closed
Closed
Closed
Merged
Merged
Closed
Closed
Closed
Closed
Closed
Merged
This was referenced Mar 16, 2018
Merged
Merged
Closed
@snyk-bot snyk-bot mentioned this pull request Aug 30, 2019
Open
@snyk-bot snyk-bot mentioned this pull request Sep 19, 2019
Open
@lock 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 lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jridgewell jridgewell jridgewell approved these changes

@loganfsmyth loganfsmyth Awaiting requested review from loganfsmyth

Assignees

No one assigned

Labels

outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

transform-es2015-modules-commonjs destructuring error

4 participants

@joshwnj @mention-bot @hzoo @jridgewell