refactor: migrate node-utils from commonjs to es6 module #121
Conversation
Codecov Report
@@ Coverage Diff @@
## master #121 +/- ##
==========================================
+ Coverage 93.2% 93.34% +0.14%
==========================================
Files 8 8
Lines 1368 1398 +30
Branches 324 324
==========================================
+ Hits 1275 1305 +30
Misses 51 51
Partials 42 42
Continue to review full report at Codecov.
|
|
No rush at all, I'll just wait until you let me know this PR is ready. I was literally thinking about this change just this morning - another legacy of the codebase being originally a pure JS package |
| node.name, | ||
| ast, | ||
| (token: any) => { | ||
| if (!token || !token.kind) { | ||
| return false; | ||
| } | ||
| return nodeUtils.getTextForTokenKind(token.kind) === '('; | ||
| return getTextForTokenKind(token.kind) === '('; | ||
| }, | ||
| ast |
There was a problem hiding this comment.
btw. this function it's really expensive to be executed, i think it's not a best way to do this xd
There was a problem hiding this comment.
Hmm interesting, I would have thought a function like that would benefit from some optimizations in the JS engine, but you might be right.
Would definitely be interesting to add some benchmarking now that things are settling down a bit.
I would not feel confident making large-scale changes without that in place (obviously not referring to something minor like this).
There was a problem hiding this comment.
no i'm not talking about execution but what is done in findFirstMatchingToken
while (previousToken) {
if (predicate(previousToken)) {
return previousToken;
}
previousToken = findNextToken(previousToken, parent, ast);
}
return undefined;
// ...
export function findNextToken(
previousToken: ts.Node,
parent: ts.Node,
ast: ts.SourceFile
): ts.Node | undefined {
return find(parent);
function find(n: ts.Node): ts.Node | undefined {
if (ts.isToken(n) && n.pos === previousToken.end) {
// this is token that starts at the end of previous token - return it
return n;
}
return firstDefined(n.getChildren(ast), (child: ts.Node) => {
const shouldDiveInChildNode =
// previous token is enclosed somewhere in the child
(child.pos <= previousToken.pos && child.end > previousToken.end) ||
// previous token ends exactly at the beginning of child
child.pos === previousToken.end;
return shouldDiveInChildNode && nodeHasTokens(child, ast)
? find(child)
: undefined;
});
}
}There was a problem hiding this comment.
there is loop in findFirstMatchingToken -> loop in findNextToken -> loop in firstDefined -> and recursion to findNextToken
this is not going to be an issue in most of scenarios, but it can be an issue in edge cases
|
🎉 This PR is included in version 18.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
benefits: tree-shaking and faster access to imports
i'm going to wait with changing converter after #120 will be finished