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

[RN][iOS] Fix warning when loading RCTUIManager and A11yManager #42734

Merged
merged 1 commit into from
Feb 4, 2024

Conversation

cipolleschi
Copy link
Contributor

@cipolleschi cipolleschi commented Jan 30, 2024

Summary:

When we fixed the race condition between A11yManager and RCTUIManager, we did it by moving the A11yManager on a background queue.
In the old architecture, this was raising a warning which our users might find confusing. Plus, that change was not aligned with what the A11yManager declared in its configuration because we are actually initializing it starting from a BG queue.

a11y_warning

With this change we anticipate the initialization of the module in a place where:

  1. We know we are in the main queue
  2. We know we are going to need it (so it is not violating the lazy load principle)
  3. We know it is safe.
    This should allow us to also remove the feature flag of RCTUIManagerDispatchAccessibilityManagerInitOntoMain because now it is safe to use the main_queue as requested by the module.

Changelog:

[iOS][Fixed] - Initialize the A11yManager in the main queue and when we need it.

Test Plan:

Tested in an app running 0.73.3, using the following configurations:

  • Old Arch | Debug
  • Old Arch | Release
  • New Arch | Debug
  • New Arch | Release

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels Jan 30, 2024
Copy link

github-actions bot commented Jan 30, 2024

Warnings
⚠️ 📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.
⚠️

Can't find the E2E test log for ci/circleci: test_e2e_ios. Job link

Generated by 🚫 dangerJS against c76fa44

@cipolleschi cipolleschi changed the base branch from main to 0.73-stable January 30, 2024 13:44
@cipolleschi cipolleschi marked this pull request as ready for review January 30, 2024 13:45
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,499,851 -8,651,144
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 9,754,834 -10,775,531
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 4c108aa
Branch: main

Copy link
Collaborator

@gabrieldonadel gabrieldonadel left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks for fixing this 👍

@jomavazquez
Copy link

Hi!
I still have the problem. Doing right now:
npx react-native@latest init MyProject --template react-native-template-typescript

which install RN 0.73.3, I get the error in the physical device and also in the simulator. I don't think it is related to RN-Reanimated library btw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner Pick Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants