Improve @babel/traverse typings #17485
Conversation
JLHwung
added
the
PR: Polish (next major) 💅
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/59858 |
| return this.replaceExpressionWithStatements(nodes); | ||
| } else if (Array.isArray(this.container)) { | ||
| return _containerInsertBefore.call(this, nodes); | ||
| return _containerInsertBefore.call(this, nodes) as NodePaths<Nodes>; |
There was a problem hiding this comment.
The new type casts in the implementation of these API are a bit unfortunate. Helps are welcome.
| } | ||
|
|
||
| export function _verifyNodeList<N extends t.Node>( | ||
| export function _verifyNodeList<N extends t.Node | null>( |
There was a problem hiding this comment.
The _verifyNodeList currently does not accept null in nodes. However, I think we should eventually accept null, because null represents a hole / empty pattern in ArrayExpression#elements or ArrayPattern#elements. The API should not have prevent users from adding them to the elements.
|
commit: |
nicolo-ribaudo
left a comment
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
This looks very nice. I left a suggestion to try removing the type casts, but even if it's not possible I very much prefer casting in @babel/traverse than in its callers.
| (parentPath.isExportDefaultDeclaration() && this.isDeclaration()) | ||
| ) { | ||
| return parentPath.insertBefore(nodes); | ||
| return parentPath.insertBefore(nodes) as NodePaths<Nodes>; |
There was a problem hiding this comment.
Would this work instead of the type case?
| return parentPath.insertBefore(nodes) as NodePaths<Nodes>; | |
| return parentPath.insertBefore<Nodes>(nodes); |
There was a problem hiding this comment.
Unfortunately not. The issue as I perceive here is that _verifyNodeList returns t.Node[] and TS considers verified nodes as a different subtype of NodeOrNodeList<t.Node> than Nodes so type casting is required.
There are also two unsound type casting in the insertAfter implementation, we might consider if we want to fix that in Babel 8.
| return this.replaceExpressionWithStatements(nodes); | ||
| } else if (Array.isArray(this.container)) { | ||
| return _containerInsertBefore.call(this, nodes); | ||
| return _containerInsertBefore.call(this, nodes) as NodePaths<Nodes>; |
There was a problem hiding this comment.
And this
| return _containerInsertBefore.call(this, nodes) as NodePaths<Nodes>; | |
| return _containerInsertBefore<Nodes>.call(this, nodes); |
There was a problem hiding this comment.
Unfortunately TS does not allow property access after TSInstantiateExpressions.
github-actions
bot
added
the
outdated
4 participants
This PR improves the typings of
NodePathmodification APIs in@babel/traverse: NamelyinsertBefore,insertAfter,unshiftContainer,pushContainerandreplaceWithMultiple. Previously the return value them were generalNodePaths, this PR introduced a few typescript mixins such that TS can infer the specific return value from the provided modification nodes. ThelistKeyofunshiftContainerandpushContaineris also narrowed to those key whose value is an array of AST nodes.Thanks to the improved typings, we can remove a few redundant type casts from their usage.