Skip to content

Conversation

kraenhansen
Copy link
Contributor

@kraenhansen kraenhansen commented Jun 8, 2024

Description

It seems to me the read of the RCT_NEW_ARCH_ENABLED environment variable is broken:

RCT_NEW_ARCH_ENABLED=0 ruby -e 'puts ENV.fetch("RCT_NEW_ARCH_ENABLED", false) ? "enabled" : "disabled"'

prints

enabled

I.e. when setting RCT_NEW_ARCH_ENABLED to 0, new_architecture_enabled? returns the string "0" which is truthy.

Platforms affected

  • Android
  • iOS
  • macOS
  • visionOS
  • Windows

Test plan

  • Run RCT_NEW_ARCH_ENABLED=0 pod install and see how the RCT_NEW_ARCH_ENABLED=1 is incorrectly appended to the "OTHER_CPLUSPLUSFLAGS" on the Xcode project.
  • Apply this fix and notice how it isn't anymore after running the command again.

@kraenhansen kraenhansen changed the title Fix new_arch_enabled? Fix new_architecture_enabled? reading RCT_NEW_ARCH_ENABLED environment variable Jun 8, 2024
Copy link
Member

@tido64 tido64 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this. Would you mind adding some test cases here as well:

def test_new_architecture_enabled?

@kraenhansen kraenhansen changed the title Fix new_architecture_enabled? reading RCT_NEW_ARCH_ENABLED environment variable fix(ios): make new_architecture_enabled? respect RCT_NEW_ARCH_ENABLED=0 Jun 10, 2024
@kraenhansen
Copy link
Contributor Author

Added a test and rebased to follow the conventional commit / contributing guidelines 👍

Copy link
Member

@tido64 tido64 left a comment

Choose a reason for hiding this comment

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

Thanks again ❤️

@tido64 tido64 enabled auto-merge (squash) June 10, 2024 14:20
@tido64 tido64 merged commit b5c1cd3 into microsoft:trunk Jun 10, 2024
@kraenhansen kraenhansen deleted the patch-1 branch June 11, 2024 07:53
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