[CDX-458] Resolve incompatibility with react 1617#48
Conversation
Radix Slot's ESM output imports "react/jsx-runtime" which fails to resolve on React 16/17 + webpack 5 (fullySpecified mode) because those React versions lack an exports map. This removes the Radix dependency entirely and replaces it with a ~70-line implementation using only React.cloneElement/forwardRef, which is compatible with React >=16.12.
esezen
changed the base branch from
main
to
no-cdx-replace-radix-slot-react16-compat
![constructor-claude-bedrock[bot]](https://avatars.githubusercontent.com/u/9828194?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This PR adds a React 16/17/18/19 compatibility test matrix (CI workflow + fixture projects) to guard against the react/jsx-runtime bare-specifier failure surfaced in CDX-458, and it is well-structured overall.
Inline comments: 4 discussions added
Overall Assessment:
|
|
||
| const root = document.getElementById('root'); | ||
| if (root) { | ||
| ReactDOM.render(<App />, root); |
There was a problem hiding this comment.
Important Issue: ReactDOM.render is the React 16/17 legacy API and works correctly here, but the React 17 fixture (test/react-compat/17/src/index.tsx) uses the identical file — including the same deprecated ReactDOM.render call. React 17 also supports ReactDOM.render, so this isn't a bug, but it means the React 17 fixture is not exercising the React 17 idiomatic entry point. More critically, if the intent is to verify that consumers can adopt the recommended patterns for each version, the React 17 entry point should match what a real React 17 consumer would write. Consider noting this distinction in a comment, or updating the React 17 fixture to document that ReactDOM.render is intentionally used (it was still the primary API in React 17).
| compat: | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| fail-fast: true |
There was a problem hiding this comment.
Suggestion: fail-fast: true means that if the React 16 cell fails, the React 17/18/19 cells are cancelled immediately. This is efficient for the CDX-458 use case (fail fast on the oldest version), but it makes it harder to see which versions are broken when multiple cells fail simultaneously (e.g. after a dependency upgrade). Consider setting fail-fast: false so all four cells always run to completion and the full compatibility picture is visible in a single CI run.
t3t3c
left a comment
t3t3c
left a comment
There was a problem hiding this comment.
I have went through everything and it looks good to me.
I have tested locally and the guard works.
What is interesting is that it breaks only on @radix-ui/react-slot@1.2.3. It works fine with @radix-ui/react-slot@1.2.5. So I think Radix managed to fix this jsx-runtime import.
esezen
deleted the
cdx-458-components-ui-resolve-incompatibility-with-react-1617-2
branch
Continuation of #47