fix(transform-fast-rest): rest parameters references with closures#798
fix(transform-fast-rest): rest parameters references with closures#798developit merged 2 commits intodevelopit:masterfrom
Conversation
|
developit
left a comment
developit
left a comment
There was a problem hiding this comment.
This is great! Curious to get your thoughts on the arrow function bit, otherwise this looks ready to go.
| binding.constant && | ||
| binding.referencePaths.length === 1 && | ||
| // arguments access requires the same function scope | ||
| getParentFunction(binding.referencePaths[0], t) === func |
There was a problem hiding this comment.
nit: there's a builtin for this (though see my other comment for why you might actually have the better approach here!)
| getParentFunction(binding.referencePaths[0], t) === func | |
| binding.referencePaths[0].getFunctionParent() === func |
|
|
||
| function getParentFunction(node, t) { | ||
| while ((node = node.parentPath)) { | ||
| if (t.isFunction(node)) { |
There was a problem hiding this comment.
Arrow functions can access arguments from the enclosing scope, so we could allow them in the tree here:
| if (t.isFunction(node)) { | |
| if (t.isFunction(node) && !t.isArrowFunctionExpression(node)) { |
There was a problem hiding this comment.
I thought about that, but if the rest parameters transformation is applied with the same criteria of the arrow function transformation there would be no gain in terms of bundlesize.
Instead if the [].slice.call is moved in the root function scope it would be cached in some cases.
What do you think?
There was a problem hiding this comment.
I've made the change and seems that without checking "arrow function" the bundle size of the test case is smaller:
With the arrow function check:
157 B: parameters-rest-closure.js.gz
115 B: parameters-rest-closure.js.br
165 B: parameters-rest-closure.esm.js.gz
124 B: parameters-rest-closure.esm.js.br
247 B: parameters-rest-closure.umd.js.gz
189 B: parameters-rest-closure.umd.js.br
With getFunctionParent:
146 B: parameters-rest-closure.js.gz
106 B: parameters-rest-closure.js.br
153 B: parameters-rest-closure.esm.js.gz
113 B: parameters-rest-closure.esm.js.br
238 B: parameters-rest-closure.umd.js.gz
177 B: parameters-rest-closure.umd.js.br
There was a problem hiding this comment.
makes sense from a Gzip perspective (repetition is essentially free), just there's a runtime cost associated with materializing arguments via slice() where it wouldn't otherwise be necessary.
2 participants
closes #797