Skip to content

refactor: use react jsx transform #1492

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

Conversation

pierrezimmermannbam
Copy link
Collaborator

Summary

Use jsx transform to get rid of all React imports

Test plan

Tests pass so the babel config should work fine and the yarn build command still runs successfully

@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (8373d6a) 97.89% compared to head (890f7ef) 97.89%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1492   +/-   ##
=======================================
  Coverage   97.89%   97.89%           
=======================================
  Files          84       84           
  Lines        5087     5087           
  Branches      786      786           
=======================================
  Hits         4980     4980           
  Misses        107      107           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mdjastrzebski
Copy link
Member

@pierrezimmermannbam does the change in this PR affects also generated code? Most of the changes files are test files, but how about the build output? Could you generate build output with and without this PR and see what is the difference there? I wonder if that could affect the users of RNTL in any way.

@@ -1,9 +1,10 @@
module.exports = {
presets: [
'@babel/preset-typescript',
'@babel/preset-react',
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this change will affect the output generated in the build folder and hence what's get packaged into NPM package. How would that affect our users.

Copy link
Member

Choose a reason for hiding this comment

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

We are shipping some minimal JSX e.g. in detectHostComponents.

@pierrezimmermannbam
Copy link
Collaborator Author

@mdjastrzebski it does affect the generated code. Compiled files now use react/jsx-runtime instead of React so it would have an impact on end users. It would be a breaking change because jsx transform was introduced with react 17 so we'd have to update the peer deps. For users that have react >= 17 it would work regardless of if they have this transform enabled in their babel config or not.

So I guess the question is do we want to drop React 16 support? I'm not too sure on this. I would assume most users have at least version 17. Also we're not running tests anymore on React 16 so we can't be sure RNTL still works fine with it and that would be the main reason to drop react 16 support imo.

On the other hand this change doesn't bring much value to the users. It's a small improvement for the DX in RNTL but that's pretty much it so it's something that we should do if we drop react 16 support but it's not really the main reason to do so

@mdjastrzebski
Copy link
Member

@pierrezimmermannbam I think we could do this in RNTL v13. I'm not event sure we support React 16 anymore due to lack of any testing, so RNTL v13 should give clean expectation in this area, perhaps our guideline could be to support the same versions as RN itself: https://reactnative.dev/versions

@mdjastrzebski
Copy link
Member

Closing for now, can be brought back with v13. #1505

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.

2 participants