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

Keep original function name when renaming scoped functions #5537

Closed
wants to merge 3 commits into from

Conversation

aaronang
Copy link
Member

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

I don't really know what the consequences are of this change. So, I am looking forward to receiving feedback.

@mention-bot
Copy link

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

@codecov
Copy link

codecov bot commented Mar 24, 2017

Codecov Report

Merging #5537 into 7.0 will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              7.0    #5537      +/-   ##
==========================================
- Coverage   85.49%   85.47%   -0.02%     
==========================================
  Files         200      200              
  Lines        9506     9507       +1     
  Branches     2701     2701              
==========================================
- Hits         8127     8126       -1     
- Misses        883      885       +2     
  Partials      496      496
Impacted Files Coverage Δ
packages/babel-helper-function-name/src/index.js 93.22% <100%> (+0.11%) ⬆️
packages/babel-traverse/src/path/context.js 84.48% <0%> (-1.73%) ⬇️

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 d0b6db1...55656c4. Read the comment docs.

Copy link
Member

@Jessidhia Jessidhia left a comment

Choose a reason for hiding this comment

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

It might be better to patch the Renamer class to preserve the name instead.

But regardless of how it is done, there is a problem with assigning the function to a var -- it's no longer hoisted 😕

_blockHoist hacks may be needed 😢

@@ -140,8 +140,7 @@ export default function ({ node, parent, scope, id }) {
if (t.isIdentifier(id)) {
const binding = scope.parent.getBinding(id.name);
if (binding && binding.constant && scope.getBinding(id.name) === binding) {
// always going to reference this method
node.id = id;
node.id = Object.assign({}, id, { name: id.loc.identifierName });
Copy link
Member

Choose a reason for hiding this comment

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

I think you can use object spread here ({ ...id, name: id.loc.identifierName })

Copy link
Member

Choose a reason for hiding this comment

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

there is t.clone(id)

@danez danez closed this Aug 31, 2017
@danez danez reopened this Aug 31, 2017
@babel-bot
Copy link
Collaborator

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/4777/

1 similar comment
@babel-bot
Copy link
Collaborator

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/4777/

@danez danez changed the base branch from 7.0 to master August 31, 2017 18:23
@loganfsmyth
Copy link
Member

I'm triaging old PR on Babel. Since this has been inactive for ages, I'm going to close it. Feel free to re-open if you think it is worth pursuing still, but I'm assuming at this point feedback won't be that useful, and it'll still be here as a reference for the issue you're trying to fix.

@loganfsmyth loganfsmyth closed this Sep 7, 2018
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wrong function name when function name overlaps with var variable (T7014)
7 participants