Lift single-expression helper restriction and use hoistable function declarations wherever possible in helpers #5706
Conversation
|
@loganfsmyth, thanks for your PR! By analyzing the history of the files in this pull request, we identified @zertosh, @existentialism and @spicyj to be potential reviewers. |
loganfsmyth
force-pushed
the
modular-helpers
branch
from
7fcbfe6
to
4ab312c
Compare
Codecov Report
@@ Coverage Diff @@
## 7.0 #5706 +/- ##
==========================================
+ Coverage 84.57% 84.67% +0.09%
==========================================
Files 282 282
Lines 9857 9935 +78
Branches 2766 2785 +19
==========================================
+ Hits 8337 8412 +75
+ Misses 1000 994 -6
- Partials 520 529 +9
Continue to review full report at Codecov.
|
loganfsmyth
force-pushed
the
modular-helpers
branch
from
4ab312c
to
34de640
Compare
| } | ||
|
|
||
| helpers.typeof = defineHelper(` | ||
| export default function _typeof(obj) { |
There was a problem hiding this comment.
They don't need the _ prefix anymore right?
Though I guess this one does as it's a keyword.
|
|
||
| helpers.typeof = defineHelper(` | ||
| export default function _typeof(obj) { | ||
| if (typeof Symbol === "function" && typeof Symbol.iterator === "symbol") { |
There was a problem hiding this comment.
I wonder if we can somehow avoid the typeof helper being defined with the typeof helper...
There was a problem hiding this comment.
Ideally we'd have better logic to not have helpers process eachother, maybe?
| if (typeof Symbol === "function" && typeof Symbol.iterator === "symbol") { | ||
| _typeof = function (obj) { return typeof obj; }; | ||
| } else { | ||
| _typeof = function (obj) { | ||
| return obj && typeof Symbol === "function" && obj.constructor === Symbol && obj !== Symbol.prototype |
There was a problem hiding this comment.
(unrelated) wouldn't instanceof be more reliable than .constructor
There was a problem hiding this comment.
Yeah not sure if this was done for perf maybe?
| return function createRawReactElement (type, props, key, children) { | ||
| var defaultProps = type && type.defaultProps; | ||
| var childrenLength = arguments.length - 3; | ||
| export default function _createRawReactElement(type, props, key, children) { |
There was a problem hiding this comment.
The helpers shouldn't need the _ anymore; if they should be prepended when inlined, then maybe change the inlining logic to ensure at least one _
There was a problem hiding this comment.
I did this so things were less likely to need to be renamed, and I still kind of feel like that is good? Especially since helpers can have multiple functions now, it's nice to make collision less likely so things keep their predefined name.
| } | ||
| } else if (!props) { | ||
| props = defaultProps || {}; | ||
| } |
There was a problem hiding this comment.
A lot of this could be simplified to
props = defaultProps || childrenLength > 0
? Object.assign({}, defaultProps, props)
: (defaultProps || {})but alas, we probably can't use Object.assign in helpers 😢
| function _defineProperties(target, props) { | ||
| for (var i = 0; i < props.length; i ++) { | ||
| var descriptor = props[i]; | ||
| descriptor.enumerable = descriptor.enumerable || false; |
There was a problem hiding this comment.
It defaults to false if omitted, so maybe this line is unnecessary
There was a problem hiding this comment.
I'd rather keep changes to helpers separate from this refactor.
| @@ -408,57 +418,53 @@ helpers.interopRequireWildcard = template(` | |||
| newObj.default = obj; | |||
There was a problem hiding this comment.
var newObj = { "default": obj }; 😆
| helpers.toConsumableArray = template(` | ||
| (function (arr) { | ||
| helpers.toConsumableArray = defineHelper(` | ||
| export default function _toConsumableArray(arr) { | ||
| if (Array.isArray(arr)) { | ||
| for (var i = 0, arr2 = Array(arr.length); i < arr.length; i++) arr2[i] = arr[i]; |
There was a problem hiding this comment.
I thought creating arrays with an initial length was a bad idea in JS
There was a problem hiding this comment.
Not sure I've heard that before. Not really looking to change the helpers in this PR.
packages/babel-helpers/src/index.js
Outdated
| }, | ||
| AssignmentExpression(child) { | ||
| const left = child.get("left"); | ||
| if (!left.isIdentifier()) return; |
There was a problem hiding this comment.
is it ok to skip destructuring here? maybe throw to make sure
There was a problem hiding this comment.
Fair, would be better to error.
loganfsmyth
force-pushed
the
modular-helpers
branch
from
34de640
to
54de7b4
Compare
|
If helpers can be imported from within other helpers, then feature request #6030 is fulfilled as well |
loganfsmyth
force-pushed
the
modular-helpers
branch
from
54de7b4
to
e737f71
Compare
loganfsmyth
force-pushed
the
modular-helpers
branch
from
e737f71
to
974f155
Compare
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/4893/ |
packages/babel-helpers/src/index.js
Outdated
| const ast = helpers[name](); | ||
| return t.file(t.program(Array.isArray(ast) ? ast : [ast])); | ||
| }; | ||
| if (!fn) throw new ReferenceError(`Unknown helper ${name}`); |
There was a problem hiding this comment.
I don't understand this. It's always a function, you just defined it.
There was a problem hiding this comment.
Yeah I 💯 put this in the wrong place, not sure what I was thinking.
packages/babel-helpers/src/index.js
Outdated
|
|
||
| traverse(file, { | ||
| ImportDeclaration() { | ||
| throw new Error(); |
There was a problem hiding this comment.
Throw the #buildCodeFrameError, please.
packages/babel-helpers/src/index.js
Outdated
|
|
||
| if (decl.isFunctionDeclaration()) { | ||
| if (!decl.node.id) { | ||
| throw new Error( |
There was a problem hiding this comment.
Throw the #buildCodeFrameError, please.
packages/babel-helpers/src/index.js
Outdated
| }, | ||
| }); | ||
|
|
||
| traverse(file, { |
There was a problem hiding this comment.
Why are we doing multiple traversals?
There was a problem hiding this comment.
The helpers are such tiny ASTs that it didn't seem like it would be an issue to traverse multiple times, especially since helpers are memoized, so at most this is called once per-helper-per-file.
packages/babel-helpers/src/index.js
Outdated
|
|
||
| return fn().expression; | ||
| for (; path; path = path.parentPath) { |
There was a problem hiding this comment.
I think you can use path.parentPath instead of path in the condition to avoid pushing and then popping (line 14) the last node.
| let exportPath; | ||
| const exportBindingAssignments = []; | ||
|
|
||
| traverse(file, { |
There was a problem hiding this comment.
Instead of doing a full traversal, you could only iterate over file.program.body.
There was a problem hiding this comment.
I wanted to create the NodePath, but yeah fair. I added a path.skip() to ignore anything isn't the root import/exports.
loganfsmyth
force-pushed
the
modular-helpers
branch
from
974f155
to
5c26c28
Compare
loganfsmyth
force-pushed
the
modular-helpers
branch
from
5c26c28
to
c15c48b
Compare
lock
bot
added
the
outdated
Essentially, I rewrote the implementation of Babel's helper system to remove the restriction of helpers being a single expression. Instead they are treated as standalone modules, and whatever their default export is will end up being the name referenced inside the user's code.
The main goal here is that now helpers don't need to be a single expression, meaning they can use function declarations more consistently, which means that this removes the need for _blockHoist on helpers.
I was in the code, so I ended up adding a general fix for ember-cli/ember-cli#7004 / #5536, but that was not the main goal.