New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve transform-destructuring
typings
#14236
Conversation
|
||
if ( | ||
t.isIdentifier(node) && | ||
t.isReferenced(node, ancestors[ancestors.length - 1]) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t.isReferenced(node, ancestors[ancestors.length - 1])
is changed to t.isReferenced(node, ancestors[ancestors.length - 1].node)
. This is a bug caught by type checker.
@@ -493,7 +43,7 @@ export default declare((api, options) => { | |||
|
|||
const specifiers = []; | |||
|
|||
for (const name of Object.keys(path.getOuterBindingIdentifiers(path))) { | |||
for (const name of Object.keys(path.getOuterBindingIdentifiers())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another bug caught by type checker.
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51391/ |
@@ -560,6 +113,7 @@ export default declare((api, options) => { | |||
path.ensureBlock(); | |||
|
|||
const block = node.body; | |||
// @ts-expect-error: ensureBlock ensures that node.body is a BlockStatement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you check if changing the ensureBlock
signature to this helps?
function ensureBlock<T extends t.Loop | t.WithStatement | t.Function | t.LabeledStatement | t.CatchClause>(
this: NodePath<T>,
): assert this is NodePath<T & { node: t.BlockStatement }>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately we can't add type assertion without breaking change because:
ensureBlock
returns at.Node
while assertion predicates requires that the function returnsvoid
.this
-type guards can only be added to a class method, though we can move it back toindex
or put it in a separate file and extends an interface (type with assertions) instead.
for (const elem of pattern.elements) { | ||
if (t.isRestElement(elem)) { | ||
return true; | ||
} | ||
} | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: these could be "compacted" to pattern.elements.some(elem => t.isRestElement(elem))
527552b
to
57535de
Compare
c309266
to
446b07e
Compare
446b07e
to
85697cf
Compare
This PR improves typings of
plugin-transform-destructuring
. I extracted these routines into a new module.They are likely to be reused in the
destructuring-private
transformer I am currently working on. In this PR they are not exposed so it can ship in patch release.