Add loose option for es2015-parameters transformation

[maurobringolf]

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

I tried to implement the loose option as described in the issue. Loose mode should replace default parameters with regular parameters and insert a conditional assignment to the default in the function body. Currently it is all wrapped in a single state.opts.loose condition. It might make more sense to merge it with the existing implementation of non-loose mode, I am not sure about that.

The tests I added and performed are still quite minimal as well, but I would like to get some feedback if I am going into the right direction.

[mention-bot]

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

[codecov]

Codecov Report

Merging #5778 into 7.0 will increase coverage by 0.03%.
The diff coverage is 95.23%.

@@            Coverage Diff            @@
##              7.0   #5778      +/-   ##
=========================================
+ Coverage   84.66%   84.7%   +0.03%     
=========================================
  Files         282     282              
  Lines        9869    9888      +19     
  Branches     2774    2778       +4     
=========================================
+ Hits         8356    8376      +20     
+ Misses        998     996       -2     
- Partials      515     516       +1

Impacted Files	Coverage Δ
...-plugin-transform-es2015-parameters/src/default.js	90.54% <95.23%> (+1.44%)	⬆️
packages/babel-traverse/src/path/context.js	87.06% <0%> (+0.86%)	⬆️
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 9e2ef0c...d857fe2. Read the comment docs.

[hzoo]

If you want to be more sure would copy over some of the tests in the current params or make some exec tests - readme example should explain we lose function arity spec behavior

[maurobringolf]

@hzoo I will add an explanation of the example in your issue to the readme and copy more tests over, thank you!

[jridgewell]

What happens if left is a complex expression? function test({a: b} = {}) {...}

[maurobringolf]

Thank you I did not consider that case yet. Since destructuring has default values on its own I only have to handle the top-level default when parameter destructuring is nested right?

I am currently trying to figure out how to write the if condition in loose mode. Would the following be correct semantics?

function test({a: { b : c} = {}} = {}) {}

Will be transformed into:

function test(a) {
  if( a === undefined ) {
    var { a : { b : c }} = {}
  }
}

Just realized that a is a confusing name for the parameter in this example since the parameter in the original function has no name, so I should generate a uuid from scope I guess?

[maurobringolf]

I copied over some more existing exec tests and realized the above is wrong because destructuring assignment is only executed when the default value is used, so I have to add an else clause. This led me to the following idea for the example above.

function test(_temp) {
  let {a: { b : c} = {}} = _temp === undefined ? {} : _temp;
}

What do you think? I am testing this now.

CORRECTION: There was an additional = {} that I removed now, see comment below.

[jridgewell]

We can go that route, but I think there's an error in the code. Should be:

  let {a: { b : c} = {}} = _temp === undefined ? {} : _temp;

Otherwise, we would have to do the defaults before the destructuring:

if (a === undefined) {
  a = {};
}
let { a: { b: c } = {} } = a;

[maurobringolf]

You are right, I am sorry about this error, but the actual source code is the way you suggested. The template I defined for this is the following:

const buildLooseDestructuredDefaultParam = template(`
  let ASSIGMENT_IDENTIFIER = PARAMETER_NAME === UNDEFINED ? DEFAULT_VALUE : PARAMETER_NAME;
`);

this will be passed the corresponding values resulting in your version of the example above.

[jridgewell]

Do we need to worry about undefined being rewritten in loose code?

[hzoo]

Yeah, should change that to be a variable and then pass scope.buildUndefinedNode()

[maurobringolf]

I have included more tests and taken destructured default parameters into account. The readme of the plugin is updated as well. Looking forward to further feedback @hzoo @jridgewell, Thank you!

[tazsingh]

An explanation of why you'd want this might be a good addition here:

This is a more performant solution as JavaScript engines will fully optimize a function that doesn't reference arguments. Please do your own benchmarking and determine if this option is the right fit for your application.

[hzoo]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters - wondering if we need tests for parameters that reference earlier params and others? I guess it's still loose mode anyway

[maurobringolf]

I think the linter is failing because I branched off 7.0 before prettier was applied? Not sure how to go about this. I also added one exec test where parameters default to earlier parameters as @hzoo suggested.

[maurobringolf]

I messed up the Git history of this branch and will have to continue this change in a new PR, sorry!