Conversation
Co-authored-by: Dunqing <29533304+Dunqing@users.noreply.github.com>
|
Hi @sapphi-red, I’m going to revert #16675, as this turned out to be more complex than I initially expected. I overlooked some logic in esbuild, where the Additionally, an invalid JSX pragma value will fail fast and break the program immediately—for example, by causing a runtime error or panic later in the transformation pipeline. In practice, users will quickly notice the issue and correct the value themselves. Given this, I think it’s better to keep the implementation simple and ignore this case for now. Let me know what you think. @sapphi-red |
|
If it's complicated, I think it's fine to keep the issue open 👍 |
There was a problem hiding this comment.
Reverting pragma validation can cause the transformer to emit syntactically invalid JavaScript when users provide invalid @jsx / @jsxFrag values, shifting failures from a clear warning to confusing downstream breakage. Even if esbuild-level AST validation is too complex, a minimal guard (or a diagnostic at the point of use) would avoid generating broken output while keeping implementation simple.
Additional notes (2)
- Compatibility |
crates/oxc_transformer/src/jsx/comments.rs:42-42
Removing validation means any pragma value (including backticks, numbers, and keywords likeimport) will be propagated into codegen. Per the PR context, this can generate syntactically invalid output (e.g.,React.createElement(`, null)) instead of failing fast with a clear diagnostic. Even if full esbuild-style AST validation is too complex, a minimal, low-cost guard here would prevent obviously-invalid tokens and avoid producing broken JS that fails downstream.
At minimum, consider rejecting values that are empty, contain whitespace, or start with a non-identifier character, and either:
-
ignore the pragma and keep defaults, or
-
emit a warning diagnostic (even if less strict than before).
-
Maintainability |
crates/oxc_transformer/src/jsx/diagnostics.rs:30-32
By removinginvalid_pragma_value(), the codebase loses a targeted warning that helped users understand why a pragma was ignored. If you decide to keep accepting arbitrary values, you may still want some diagnostic when the resulting transform would become invalid (or when codegen later fails), otherwise users will see confusing downstream errors.
If the revert is intentional, ensure there is an alternative path that produces actionable errors when invalid pragma values cause transform/codegen failures.
Summary of changes
Summary
This diff reverts JSX pragma validation introduced in a prior change.
crates/oxc_transformer/src/jsx/comments.rs
- Removed imports and logic that validated
@jsx/@jsxFragvalues. update_options_with_commentno longer acceptsctx: &TransformCtxand no longer emits diagnostics for invalid pragma values.- Deleted
is_valid_pragma_value()(identifier/keyword-based validation derived from esbuild).
crates/oxc_transformer/src/jsx/diagnostics.rs
- Removed
invalid_pragma_value()warning diagnostic.
Conformance tests
- Deleted fixtures for
invalid-jsxandinvalid-jsx-frag. - Updated snapshot counts accordingly (
Passed: 201/332, and react-jsx suite count changes).
There was a problem hiding this comment.
Pull request overview
This PR reverts commit 853c20d which added validation for JSX pragma values in comments. The revert removes all validation logic that rejected invalid pragma values (like backticks, numbers, reserved keywords) and diagnostic warnings, returning to the previous behavior of accepting any pragma value without validation.
Key Changes
- Removed
is_valid_pragma_value()validation function that checked for valid JavaScript identifiers - Removed
invalid_pragma_value()diagnostic warning - Removed test fixtures for invalid pragma cases
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
crates/oxc_transformer/src/jsx/comments.rs |
Removed validation logic and helper function; pragma values are now accepted without checks |
crates/oxc_transformer/src/jsx/diagnostics.rs |
Removed invalid_pragma_value() diagnostic function |
tasks/transform_conformance/tests/babel-plugin-transform-react-jsx/test/fixtures/invalid-jsx/ |
Removed test fixture for invalid @jsx pragma |
tasks/transform_conformance/tests/babel-plugin-transform-react-jsx/test/fixtures/invalid-jsx-frag/ |
Removed test fixture for invalid @jsxFrag pragma |
tasks/transform_conformance/snapshots/oxc.snap.md |
Updated test count from 203/334 to 201/332 reflecting removed tests |
The revert is complete and consistent - all validation code, diagnostics, unused imports, and associated tests have been properly removed. The code will now accept any pragma value and use it as-is in transformations, potentially producing invalid JavaScript output when invalid pragma values are provided.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CodSpeed Performance ReportMerging #16793 will not alter performanceComparing Summary
Footnotes
|
Reverts commit 853c20d which added JSX pragma validation. The validation rejected invalid pragma values (backticks, numbers, keywords) and emitted warnings while falling back to defaults.
Changes
crates/oxc_transformer/src/jsx/comments.rs: Removedis_valid_pragma_value()function and validation checks for@jsxand@jsxFragpragmas. Invalid pragma values are now accepted without validation.crates/oxc_transformer/src/jsx/diagnostics.rs: Removedinvalid_pragma_value()diagnostic.Test fixtures: Removed
invalid-jsxandinvalid-jsx-fragtest cases.Behavior
Invalid pragma values are now used as-is in transformation:
Transforms to invalid code:
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.