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
fix: traverse.visitors.merge
unable to handle enter,exit
#15593
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/54580/ |
key: string, | ||
value: unknown, | ||
): VisitorHandler | void { | ||
function assertVisitorHandler(key: string, value: unknown) { |
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.
Can we add a return assertion here? Like asserts value is VisitorHandler
.
value: unknown, | ||
): VisitorHandler | void { | ||
function assertVisitorHandler(key: string, value: unknown) { | ||
if (key === "_exploded" || key === "_verified") return; |
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.
We explicitly disallow catch-all enter
and exit
in Babel plugins:
babel/packages/babel-core/src/config/validation/plugins.ts
Lines 43 to 49 in 2db3f55
if (obj.enter || obj.exit) { | |
throw new Error( | |
`${msg( | |
loc, | |
)} cannot contain catch-all "enter" or "exit" handlers. Please target individual nodes.`, | |
); | |
} |
If key
is _exploded
, then value
is a visitor processed by explode
. In this case, I think we should move the catch-all check before the visitor shape check, so we don't have to handle _exploded
and _verified
in assertVisitorHandler
.
@@ -222,9 +224,43 @@ export function merge( | |||
for (const type of Object.keys(visitor) as (keyof Visitor)[]) { |
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.
If visitor
has enter
or exit
, then it is a catch all visitor. In this case we don't have to go through its properties, instead we can call wrapWithStateOrWrapper
on .enter
and .exit
and push it to the rootVisitor. In this approach we can set _exploded
and _verified
directly.
import _traverse, { visitors } from "../lib/index.js"; | ||
const traverse = _traverse.default || _traverse; | ||
|
||
describe("visitors", () => { |
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: should be visitors.merge
, or add a new nested describe
block.
Superseded by #15702 |
I'm not sure if this will be a breaking change, although it's a bugfix, since the
state
parameter didn't supportenter
andexit
at all before then.