Conversation
liuxingbaoyu
commented
Oct 3, 2025
| Q | A |
|---|---|
| Fixed Issues? | Fixes #17537 |
| Patch: Bug Fix? | √ |
| Major: Breaking Change? | |
| Minor: New Feature? | |
| Tests Added + Pass? | √ |
| Documentation PR Link | |
| Any Dependency Changes? | |
| License | MIT |
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/60136 |
|
commit: |
|
Thanks for jumping on this so quickly! I noticed there's a slight error when a JSX element with a comment inside it is nested in another JSX element, looks like it gets hoisted a bit too far: <div><Trans {.../*i18n*/{a:1}}/></div>becomes import { jsx as _jsx } from "react/jsx-runtime";
/*#__PURE__*/_jsx("div",
/*i18n*/
{
children: /*#__PURE__*/_jsx(Trans, {
a: 1
})
}); |
| i?: number, | ||
| ) { | ||
| if (i === 0) { | ||
| leadingComments = undefined; |
There was a problem hiding this comment.
Should we also reset leadingComments when i is undefined?
There was a problem hiding this comment.
We manually reset i when it is undefined.
| }, | ||
| }; | ||
|
|
||
| let leadingComments: t.Comment[] | undefined; |
There was a problem hiding this comment.
Personally, I think it’s a bit hacky to intertwine the leadingComments logic with the attribute accumulator. This approach doesn’t address how we should handle the trailingComments of a JSXSpreadAttribute, such as in <div {...{ id: 'hello' }/*i18n*/} />.
Could we record the source node where the comments originate and the target node to which they will be attached? Then we could call the t.inheritsComments API, which was designed for comment adjustment.
| } | ||
| if (t.isJSXSpreadAttribute(attribute.node)) { | ||
| const arg = attribute.node.argument; | ||
| leadingComments ||= arg.leadingComments; |
There was a problem hiding this comment.
We should merge the arrays if both are defined. Can you also add tests with multiple attributes?
There was a problem hiding this comment.
This commit is old. :)
However, the new logic is similar.
We only handled JSXSpreadAttribute, and JSX seems to only allow one JSXSpreadAttribute per element.
So it seems there's no problem.
I added a few tests, not sure if this is what you mean.
There was a problem hiding this comment.
Oh sorry I didn't notice I had a stale comment.
8bfdfcd to
fae117b
Compare