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 bug - undefined reference for export declaration #3629

Merged
merged 1 commit into from
Aug 31, 2016

Conversation

boopathi
Copy link
Member

@boopathi boopathi commented Aug 4, 2016

+ (Fix https://phabricator.babeljs.io/T7534)
+ Export declaration class/function/var ids now add the export
declaration path as the referenced one.
@codecov-io
Copy link

codecov-io commented Aug 4, 2016

Current coverage is 88.61% (diff: 100%)

Merging #3629 into master will not change coverage

@@             master      #3629   diff @@
==========================================
  Files           188        188          
  Lines         15486      15486          
  Methods        1838       1838          
  Messages          0          0          
  Branches       4122       4122          
==========================================
  Hits          13723      13723          
  Misses         1763       1763          
  Partials          0          0          

Powered by Codecov. Last update 897f553...0223dda

@hzoo
Copy link
Member

hzoo commented Aug 4, 2016

Oh interesting - makes sense... wonder if we can add a simple test for this one? (Yeah I know there aren't many for traverse)

Can merge and add as a todo later too

@hzoo hzoo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Aug 4, 2016
@hzoo hzoo added this to the Next Patch milestone Aug 4, 2016
let declar = node.declaration;
if (t.isClassDeclaration(declar) || t.isFunctionDeclaration(declar)) {
let id = declar.id;
if (!id) return;

let binding = scope.getBinding(id.name);
if (binding) binding.reference();
if (binding) binding.reference(path);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just not sure if it should be binding.path or path ... I guess it should be path, or both path and binding.path...

Copy link
Member

@hzoo hzoo Aug 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so path points to the "ExportDefaultDeclaration" and binding.path points to the delcaration of the export, the "ClassDeclaration"

sounds like path based on this and this?

unless it's supposed to be the path to the identifier so that would be path.get('declaration.id')?

Oh yeah it errors with

Unexpected ExportDefaultDeclaration. Expected an Identifier
    at Mangler.renameNew (./babel-plugin-minify-mangle-names/lib/index.js:193:19)
          if (!path.isIdentifier()) {
            // if this occurs, then it is
            // probably an upstream bug (in babel)
            throw new Error("Unexpected " + path.node.type + ". Expected an Identifier");
          }

so maybe path.get('declaration.id') for the first and path.get('declaration.declarations')[i] for the 2nd

@hzoo hzoo merged commit 477a72a into babel:master Aug 31, 2016
@boopathi boopathi deleted the export-binding-var branch August 31, 2016 18:43
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 7, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 7, 2019
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.

3 participants