Skip to content
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

Transform: Tab \t in JSX should be trimmed #6638

Closed
magic-akari opened this issue Oct 17, 2024 · 4 comments · Fixed by #6639
Closed

Transform: Tab \t in JSX should be trimmed #6638

magic-akari opened this issue Oct 17, 2024 · 4 comments · Fixed by #6639
Labels
C-bug Category - Bug

Comments

@magic-akari
Copy link
Contributor

OXC playground

@overlookmotel
Copy link
Contributor

I think this should be fixed in parser (not transformer). I don't think we should have had nodes like this in the AST in the first place:

JSXText {
    span: Span { start: 68, end: 72 },
    value: "\n\t\t\t",
}

@Boshen can you confirm I am right about that?

@magic-akari
Copy link
Contributor Author

I think this should be fixed in parser (not transformer). I don't think we should have had nodes like this in the AST in the first place:

JSXText {
    span: Span { start: 68, end: 72 },
    value: "\n\t\t\t",
}

@Boshen can you confirm I am right about that?

When I attempted to fix the issue, I suspected the parser.
But upon examining the AST outputs from both TypeScript and Babel, I discovered that they were both present in the AST.

When I tried to fix it, I suspected the parser might be the issue.
However, after reviewing the AST outputs from both TypeScript and Babel, I realized that they arre preserved in the AST.

overlookmotel pushed a commit that referenced this issue Oct 17, 2024
@overlookmotel overlookmotel reopened this Oct 18, 2024
@overlookmotel
Copy link
Contributor

Re-opening as would like @Boshen's view on whether parser should be producing these JSXText elements which contain only whitespace.

If I remember right, we decided that our support for JSX is limited to React JSX (and React-like JSX, e.g. Vue), rather than attempting to follow the full "abstract" JSX spec. Therefore, even if JSX spec says these JSXText nodes should be present, I don't think that's relevant.

In React JSX (and ditto Vue) whitespace in these positions is not semantically meaningful. We don't use a Concrete Syntax Tree so, like other semantically-meaningless whitespace, personally I don't think this should appear in the AST.

It's not purely an academic question. In typical React/Vue/etc code, our AST will contain a lot of these pointless JSXText nodes. It may well has a measurable perf impact. @magic-akari's simple example has 11 JSXText nodes in it.

@Boshen
Copy link
Member

Boshen commented Oct 18, 2024

The parser is correct according to the spec https://facebook.github.io/jsx/#prod-JSXText

The transformer is responsible for how to deal with these whitespaces, which is also correct, because our jsx transform is correct.

@Boshen Boshen closed this as completed Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category - Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants