Skip to content

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

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Copilot
Copy link

@Copilot Copilot AI commented Jun 28, 2025

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:

  1. As trailing comments during variable declaration processing
  2. As JSX text content during JSX element processing

Example:

Input:

const jsx = <App>/* no */{/* 1 */ 123 /* 2 */}/* no */</App>;

Before (buggy output):

var jsx = <App> /* no *//* no */{/* 1 */123 /* 2 */} /* no *//* no */</App>;

After (fixed output):

var jsx = <App>/* no */{123}/* no */</App>;

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:

  • Tracks the current JSX element being processed via currentJsxElement
  • Checks if trailing comment positions are within JSX element ranges
  • Skips duplicate emission when JSX elements contain comment-only text nodes

Changes

  • Modified src/compiler/emitter.ts to add JSX-aware comment emission logic
  • Added comprehensive test case to prevent regression
  • Preserves all existing JSX functionality while fixing the specific duplication issue

Testing

  • ✅ New test case validates the fix
  • ✅ All existing JSX comment tests pass (commentsOnJSXExpressionsArePreserved)
  • ✅ All JSX element tests pass (44 tests)
  • ✅ Lint and format checks pass

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.

@Copilot Copilot AI changed the title [WIP] JSX comment duplicated Fix JSX comment duplication in preserve mode Jun 28, 2025
@Copilot Copilot AI requested a review from RyanCavanaugh June 28, 2025 05:13
Copilot finished work on behalf of RyanCavanaugh June 28, 2025 05:13
Copy link
Owner

@RyanCavanaugh RyanCavanaugh left a 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.

Copy link
Author

Copilot AI commented Jun 28, 2025

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. W...

You're absolutely right. The issue was that JSX text content like /* no */ was being incorrectly treated as comments by the comment iteration system.

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 /* no */ inside JSX elements is treated purely as JSX text content, not as comments to be duplicated. Fix is in commit 0cb049f.

@Copilot Copilot AI requested a review from RyanCavanaugh June 28, 2025 07:36
Copilot finished work on behalf of RyanCavanaugh June 28, 2025 07:36
Copy link
Owner

@RyanCavanaugh RyanCavanaugh left a 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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// @jsx: preserve
// @jsx: preserve,react,react-jsx

Copy link
Author

Copilot AI commented Jun 28, 2025

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 t...

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 - emitLeadingCommentsOfNode and emitTrailingCommentsOfNode check node.kind === SyntaxKind.JsxText and skip comments. I'll refactor to pass the current node context to the comment emission functions instead of the expensive pre-walk approach.

…tead of expensive pre-walk

Co-authored-by: RyanCavanaugh <[email protected]>
@Copilot Copilot AI requested a review from RyanCavanaugh June 28, 2025 16:50
Copilot finished work on behalf of RyanCavanaugh June 28, 2025 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSX comment duplicated
2 participants