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

Fix 5768 #5811

Merged
merged 2 commits into from Jun 27, 2017
Merged

Fix 5768 #5811

merged 2 commits into from Jun 27, 2017

Conversation

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

- adds a failing test based on description in babel#5768
- handles ObjectPattern and ArrayPattern
@mention-bot
Copy link

@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.

@codecov
Copy link

codecov bot commented Jun 1, 2017

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
Member

hzoo commented Jun 27, 2017

Nice work @joshwnj!

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

hzoo commented Jun 27, 2017

Hm realized this is also against master instead of 7.0 heh

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

hzoo commented Jun 27, 2017

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

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

That'd be great if you did it!

@joshwnj
Copy link
Contributor Author

joshwnj commented Jun 27, 2017

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

@joshwnj
Copy link
Contributor Author

joshwnj commented Jun 27, 2017

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Development

Successfully merging this pull request may close these issues.

transform-es2015-modules-commonjs destructuring error
4 participants