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

revert back to the old JSX transform #2122

Merged
merged 8 commits into from
Jul 2, 2024
Merged

Conversation

mayank99
Copy link
Contributor

@mayank99 mayank99 commented Jun 28, 2024

Changes

This reverts the new JSX transform added in #2102 that shipped with 3.12.0.

This is because we immediately started receiving reports that it doesn't work for React 17 users that use Webpack 5. I did some digging and found that the React team was made aware of this issue back in 2020, and they willingly decided not to fix it until v18 (bad decision from them imo). And yes, this means their lofty claims about the new transform being supported in older React versions are untrue.

The main reason for using the new JSX transform was that it was "required" for React 19. However, upon reading closely, it seems like it will only raise a warning, not an error, so it is in fact not "required". Further, I tested iTwinUI 3.11.3 (which uses the old JSX transform) with React 19 RC and did not even see any warnings.

This does mean that we are stuck with the older, less performant JSX runtime for the foreseeable future. I'm making this change reluctantly, but it seems to work in React 17-19 and is the least frustrating outcome for our users (the majority of whom are still using React 17). I also tried to find a "best of both worlds" solution for the last couple days, with no luck.

I've strategically made the smallest change possible instead of fully reverting #2102. Only two source files (StatusIconMap.tsx and selectionColumn.tsx) needed to be changed to add a missing React import. Everything else (mainly test files) continue to use the new transform, since these are not relevant for our users. I also made some small unrelated changes along the way.

Testing

Veridied that the build output now contains React.createElement everywhere.

Docs

N/A. Added patch changeset to be released in 3.12.1

@mayank99 mayank99 self-assigned this Jun 28, 2024
@mayank99 mayank99 requested review from a team as code owners June 28, 2024 16:47
@mayank99 mayank99 requested review from r100-stack and Ben-Pusey-Bentley and removed request for a team June 28, 2024 16:47
@mayank99 mayank99 marked this pull request as draft June 28, 2024 18:35
@Ben-Pusey-Bentley
Copy link
Contributor

I think the vitest.config.ts file needs esbuild.jsx to be set to 'auotmatic' in order to build our test.tsx unit test files with the new JSX transform. Docs: https://vitejs.dev/config/shared-options.html#esbuild https://esbuild.github.io/api/#jsx.

Copy link
Contributor Author

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

I think the vitest.config.ts file needs esbuild.jsx to be set to 'auotmatic' in order to build our test.tsx unit test files with the new JSX transform.

Thanks, fixed. I was trying to figure out how to make it use the tsconfig.test.json file but it seems like that's not possible.

@mayank99 mayank99 marked this pull request as ready for review June 30, 2024 21:01
Copy link
Contributor

@Ben-Pusey-Bentley Ben-Pusey-Bentley left a comment

Choose a reason for hiding this comment

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

Nit: The Docs section of the PR description should acknowledge the changeset. Otherwise, LGTM.

@mayank99 mayank99 merged commit d47d9bd into main Jul 2, 2024
16 checks passed
@mayank99 mayank99 deleted the mayank/revert-jsx-transform branch July 2, 2024 14:04
@imodeljs-admin imodeljs-admin mentioned this pull request Jul 2, 2024
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.

3 participants