Skip to content

fix(transformer/jsx): fix parsing JSX pragma comments#10983

Merged
graphite-app[bot] merged 1 commit intomainfrom
05-12-fix_transformer_jsx_fix_parsing_jsx_pragma_comments
May 13, 2025
Merged

fix(transformer/jsx): fix parsing JSX pragma comments#10983
graphite-app[bot] merged 1 commit intomainfrom
05-12-fix_transformer_jsx_fix_parsing_jsx_pragma_comments

Conversation

@overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented May 12, 2025

Fixes #10955.

Allow multiple JSX pragmas in a single comment, or even in the same line. e.g.

/* @jsx h @jsxRuntime classic */

// @jsx h @jsxRuntime classic

/**
  * @jsx h
  * @jsxRuntime classic
  */

Also, allow pragmas to be anywhere in the comment, including surrounded by unrelated text.

// Unrelated text @jsx h Unrelated text @jsxRuntime classic

/**
  * Unrelated text.
  * @jsx h
  * Unrelated text.
  * @jsxRuntime classic
  */

This is more liberal than Babel, which has a test specifically to outlaw placing pragmas amongst other text in a comment - this test is disabled in this PR.

Instead, we're aligning with ESBuild (ESBuild playground) which seems to allow a @jsx pragma anywhere in the text of the comment.

@github-actions github-actions bot added the A-transformer Area - Transformer / Transpiler label May 12, 2025
Copy link
Member Author


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions github-actions bot added the C-bug Category - Bug label May 12, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented May 12, 2025

CodSpeed Instrumentation Performance Report

Merging #10983 will degrade performances by 3.51%

Comparing 05-12-fix_transformer_jsx_fix_parsing_jsx_pragma_comments (27a21a7) with main (287b3c3)

Summary

❌ 1 regressions
✅ 35 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
transformer[antd.js] 40.8 ms 42.3 ms -3.51%

@overlookmotel overlookmotel marked this pull request as ready for review May 12, 2025 18:46
@overlookmotel overlookmotel requested a review from Dunqing as a code owner May 12, 2025 18:46
@overlookmotel
Copy link
Member Author

transformer[antd.js] 40.8 ms 42.3 ms -3.53%

I'll try to address this perf regression in a follow-up.

@Dunqing
Copy link
Member

Dunqing commented May 13, 2025

transformer[antd.js] 40.8 ms 42.3 ms -3.53%

I'll try to address this perf regression in a follow-up.

Weird, shouldn't have any performance impact in this benchmark, as it applies when the source type is JSX. Could you investigate why this is happening?

if program.source_type.is_jsx() {
jsx::update_options_with_comments(
&program.comments,
&mut self.typescript,
&mut self.jsx,
&self.ctx,
);
}

@Dunqing Dunqing added the 0-merge Merge with Graphite Merge Queue label May 13, 2025
Copy link
Member

Dunqing commented May 13, 2025

Merge activity

Fixes #10955.

Allow multiple JSX pragmas in a single comment, or even in the same line. e.g.

```js
/* @jsx h @jsxRuntime classic */

// @jsx h @jsxRuntime classic

/**
  * @jsx h
  * @jsxRuntime classic
  */
```

Also, allow pragmas to be anywhere in the comment, including surrounded by unrelated text.

```js
// Unrelated text @jsx h Unrelated text @jsxRuntime classic

/**
  * Unrelated text.
  * @jsx h
  * Unrelated text.
  * @jsxRuntime classic
  */
```

This is more liberal than Babel, which [has a test](https://github.com/babel/babel/blob/226318d54eaff275500b2723c46ee608c0632df0/packages/babel-plugin-transform-react-jsx/test/fixtures/react/should-not-allow-jsx-pragma-to-be-anywhere-in-comment/input.js) specifically to outlaw placing pragmas amongst other text in a comment - this test is disabled in this PR.

Instead, we're aligning with ESBuild ([ESBuild playground](https://esbuild.github.io/try/#dAAwLjI1LjQALS1sb2FkZXI9anN4IC0tanN4PWF1dG9tYXRpYwAvLyBAanN4IFNvbWV0aGluZyBibGFoIGJsYWggQGpzeFJ1bnRpbWUgY2xhc3NpYwo8YmxhaC8+Cg)) which seems to allow a `@jsx` pragma anywhere in the text of the comment.
@graphite-app graphite-app bot force-pushed the 05-12-fix_transformer_jsx_fix_parsing_jsx_pragma_comments branch from 0990df0 to 27a21a7 Compare May 13, 2025 00:29
@graphite-app graphite-app bot merged commit 27a21a7 into main May 13, 2025
26 checks passed
@graphite-app graphite-app bot deleted the 05-12-fix_transformer_jsx_fix_parsing_jsx_pragma_comments branch May 13, 2025 00:35
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label May 13, 2025
@overlookmotel
Copy link
Member Author

overlookmotel commented May 13, 2025

Weird, shouldn't have any performance impact in this benchmark, as it applies when the source type is JSX. Could you investigate why this is happening?

SourceType::from_path interprets .js extension as JSX, so antd.js benchmark is treated as JSX.

graphite-app bot pushed a commit that referenced this pull request May 14, 2025
…11001)

Use `memchr` for finding `@` when parsing JSX pragmas from comments.

This wins back most (but not all) of the perf loss of #10983 on `antd.js` benchmark, and preserves the perf gain of #10983 on `cal.com.tsx` benchmark.

Interestingly, using `memchr` to search just for `@` and then checking next 3 bytes are `jsx` separately is measurably faster than using `memchr::memmem::Finder` to search for `@jsx`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-transformer Area - Transformer / Transpiler C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

transformer: multiple jsx pragma in a single line is not parsed as multiple values

2 participants