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

Inline parameters #5751

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Inline parameters #5751

wants to merge 10 commits into from

Conversation

peey
Copy link
Contributor

@peey peey commented May 19, 2017

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

Add path.inlineParameters() for use in es2015-parameters and object-rest-spread plugins

  • Moves all the function parameters to body in an arrayPattern initializer,
  • Uses block scoping for the case of function declaration as a default argument or in case of a var redeclaration of a parameter binding in function body

Not Added:

  • A way to convert the rest parameter. Since code for handling this already exists in es2015-parameters plugin, I think that's a better place for that logic.

peey added 4 commits May 19, 2017 20:40
Similar to what path.find does for ancestors, path.findChild finds the
first match for a given type and an optional predicate function
Moves all the parameters to body in an arrayPattern initializer. Uses block
scoping for the case of function declaration as a default argument.
@mention-bot
Copy link

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

@codecov
Copy link

codecov bot commented May 19, 2017

Codecov Report

Merging #5751 into 7.0 will increase coverage by 0.1%.
The diff coverage is 95.83%.

Impacted file tree graph

@@            Coverage Diff            @@
##              7.0    #5751     +/-   ##
=========================================
+ Coverage   85.15%   85.25%   +0.1%     
=========================================
  Files         284      285      +1     
  Lines        9953    10025     +72     
  Branches     2776     2789     +13     
=========================================
+ Hits         8475     8547     +72     
+ Misses        977      974      -3     
- Partials      501      504      +3
Impacted Files Coverage Δ
packages/babel-traverse/src/path/index.js 83.13% <ø> (ø) ⬆️
packages/babel-traverse/src/path/family.js 78.37% <100%> (+1.9%) ⬆️
packages/babel-traverse/src/path/parameters.js 95.23% <95.23%> (ø)
...bel-plugin-transform-es2015-classes/src/vanilla.js 91.06% <0%> (+0.42%) ⬆️
packages/babel-traverse/src/path/context.js 86.2% <0%> (+1.72%) ⬆️

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 36ab72f...d8e7f66. Read the comment docs.

@peey peey force-pushed the inline-parameters branch 3 times, most recently from 5369574 to 104cef3 Compare May 19, 2017 18:39
peey added 2 commits May 20, 2017 01:01
To be able to use the process variable in tests to ignore node versions
without support for language features required by the test
@@ -0,0 +1,11 @@
if (parseInt(process.version.slice(1)) < 6) process.exit();
Copy link
Member

Choose a reason for hiding this comment

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

Would be great if you can update to use the newly merged minNodeVersion option and get rid of the node check & eval here and a few other tests in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for adding that feature! I'll update my code asap :)

@jridgewell
Copy link
Member

Why does this require an array destructuring?

@peey
Copy link
Contributor Author

peey commented May 26, 2017

@jridgewell the idea is that instead of duplicating all destructing and rest/spread logic (in argument defaults) for parameters, we have a general function which would inline the parameters into the function body.

Then the code to handle array destructing and rest/spread could do the further processing.

@peey
Copy link
Contributor Author

peey commented Jun 9, 2017

@loganfsmyth need your thoughts on this if you still think this is a good idea. After this we can proceed on incorporating it into object-rest-spread and the parameters transform to solve a lot of issues.

@hzoo hzoo requested a review from loganfsmyth June 10, 2017 13:29
@danez danez closed this Aug 31, 2017
@danez danez reopened this Aug 31, 2017
@danez danez changed the base branch from 7.0 to master August 31, 2017 18:19
@loganfsmyth loganfsmyth removed their request for review February 16, 2021 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants