Skip to content

Conversation

@johnf
Copy link
Contributor

@johnf johnf commented Mar 25, 2024

The previous approach could fail in a monorepo where both modules could exist in a hoisted node_modules

Description

I discovered an issue with selecting which version of the react native module to include.

The current code checks if @react-native/gradle-plugin exists (RN >= 0.72) and usesit if it does.

This logic fails in a monorepo situation where both libraries could exist for different packages.

This change explicitly checks the react native version and loas the appropriate library.

This is on the path to helping me solve #1932

Platforms affected

  • Android
  • iOS
  • macOS
  • visionOS
  • Windows

Test plan

I'm currently testing this against my code base. Easiest way to reproduce

  • Create a yarn workspace
  • Create to react-native-test-apps inside
  • One on 0.73 and on one 0.70
  • Make sure you can build both

@johnf johnf requested review from kelset and tido64 as code owners March 25, 2024 10:13
@johnf johnf mentioned this pull request Mar 25, 2024
6 tasks
@kelset
Copy link
Contributor

kelset commented Mar 25, 2024

hey @johnf one quick headsup, you are missing the : so the commit linter fails

Copy link
Contributor

@kelset kelset left a comment

Choose a reason for hiding this comment

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

aside from the commit lint issue, the change is solid and respect existing approch, LGTM 👍

one small nit is that I'm not sure we'd recommend having multiple RNTA apps in the same codebase on different RN versions, but we can explore more that conversation here (#1932)

The previous approach could fail in a monorepo where both modules could
exist in a hoisted node_modules
@johnf
Copy link
Contributor Author

johnf commented Mar 25, 2024

fixed

@tido64 tido64 enabled auto-merge (squash) March 25, 2024 20:52
@tido64
Copy link
Member

tido64 commented Mar 25, 2024

@johnf, this is a good fix. Thanks ❤️

@tido64 tido64 disabled auto-merge March 25, 2024 20:57
@tido64 tido64 enabled auto-merge (squash) March 25, 2024 20:59
@tido64 tido64 merged commit 211891f into microsoft:trunk Mar 25, 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