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

handle deopt case in builtins properly #533

Merged
merged 5 commits into from
May 22, 2017
Merged

handle deopt case in builtins properly #533

merged 5 commits into from
May 22, 2017

Conversation

vigneshshanmugam
Copy link
Member

@vigneshshanmugam vigneshshanmugam commented May 11, 2017

);
t.traverseFast(node, subNode => {
if (t.isFunction(subNode)) {
funcNode = subNode;
Copy link
Member Author

Choose a reason for hiding this comment

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

will try this with generators if we can avoid full traversal.

@codecov
Copy link

codecov bot commented May 11, 2017

Codecov Report

Merging #533 into master will decrease coverage by 0.05%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #533      +/-   ##
=========================================
- Coverage   83.26%   83.2%   -0.06%     
=========================================
  Files          34      34              
  Lines        2539    2549      +10     
  Branches      908     915       +7     
=========================================
+ Hits         2114    2121       +7     
- Misses        254     256       +2     
- Partials      171     172       +1
Impacted Files Coverage Δ
packages/babel-plugin-minify-builtins/src/index.js 89.79% <77.77%> (-2.25%) ⬇️

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 e2710cf...b348cb8. Read the comment docs.

@vigneshshanmugam
Copy link
Member Author

@boopathi This is ready for review now and all the edge cases and included in the test.

return path;
}
}
return void 0;
Copy link
Member

Choose a reason for hiding this comment

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

nit: this statement is not required

if (!(lastCommon.isProgram() || funParent.isProgram())) {
segments.set(funParent, paths);
return;
}
Copy link
Member

@boopathi boopathi May 18, 2017

Choose a reason for hiding this comment

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

in both the above conditions, lastCommon.isProgram() should be false. So, maybe something like this -

let fnParent;
if (!lastCommon.isProgram()) {
  if (lastCommon.isFunction()) {
    segments.set(lastCommon, paths);
    return;
  } else if (!(fnParent = lastCommon.getFunctionParent()).isProgram()) {
    segments.set(fnParent, paths);
    return;
  }
}

@boopathi boopathi added the Tag: Bug Fix Pull Request fixes a bug label May 19, 2017
@boopathi
Copy link
Member

Let's also handle the case where the arrowFn body isn't a block statement.

for example:

function foo() {
  const x = () => Math.max(Math.max(a,b), Math.max(Math.floor(b), Math.floor(c)))
}

@vigneshshanmugam
Copy link
Member Author

vigneshshanmugam commented May 22, 2017

Good catch. de-opting for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tag: Bug Fix Pull Request fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hoisted or aliased variable issue with webpack babilified code fails at runtime on SystemJS
2 participants