-
Notifications
You must be signed in to change notification settings - Fork 3
Fix JSX comment duplication in preserve mode #13
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: RyanCavanaugh <[email protected]>
Co-authored-by: RyanCavanaugh <[email protected]>
Co-authored-by: RyanCavanaugh <[email protected]>
Co-authored-by: RyanCavanaugh <[email protected]>
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.
Good testcase but this fix is a little off. I think the problem is that we're treating the /* no */
as a comment at all - it's not a comment, it's just JSX text, and is not a comment whatsoever. Whatever part of the system thinks this is a comment is wrong - track that down, and fix it.
… emission Co-authored-by: RyanCavanaugh <[email protected]>
You're absolutely right. The issue was that JSX text content like Fixed by modifying the comment emission system to exclude positions that fall within JSX text ranges. The solution pre-collects all JSX text ranges when processing a source file, then filters out any comment positions that fall within those ranges during comment emission. This ensures that |
Co-authored-by: RyanCavanaugh <[email protected]>
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.
This is making more sense, but I'm still a little skeptical about this fix. We shouldn't need to pre-walk the whole tree like this. Why doesn't this same problem happen with comment-like text inside template literal strings, e.g.
const foo = `bar/*not-duplicated*/${32}`;
We must already have logic that can detect preceding comment ranges like that. Re-use or mimic that logic here instead of putting an expensive full-file walk in place.
@@ -0,0 +1,3 @@ | |||
// @jsx: preserve |
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.
// @jsx: preserve | |
// @jsx: preserve,react,react-jsx |
You're absolutely right about the pre-walk being expensive. Looking at template literals, I see they don't have this issue because their content isn't parsed as separate comment tokens in the first place. The existing code already has the right pattern - |
…tead of expensive pre-walk Co-authored-by: RyanCavanaugh <[email protected]>
Problem
When using JSX preserve mode, comments that appear as JSX text nodes were being duplicated in the output. This happened because the same comments were being emitted twice:
Example:
Input:
Before (buggy output):
After (fixed output):
Solution
Added logic in
forEachTrailingCommentToEmit
to detect when trailing comment positions fall within JSX elements that contain comment-only text children. In these cases, we skip the trailing comment emission since the JSX processor will handle these comments directly.The fix:
currentJsxElement
Changes
src/compiler/emitter.ts
to add JSX-aware comment emission logicTesting
commentsOnJSXExpressionsArePreserved
)Fixes #11.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.