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

(iOS) - NativeCommands fail in ref functions if batchRenderingUpdatesInEventLoop is active #46330

Open
RyanCommits opened this issue Sep 4, 2024 · 10 comments
Labels
Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. Newer Patch Available Platform: iOS iOS applications. Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules)

Comments

@RyanCommits
Copy link

RyanCommits commented Sep 4, 2024

Description

Problem

Starting in version 0.74.1 and above, due to this change, the batchRenderingUpdatesInEventLoop feature flag is turned ON. This causes NativeCommands called in ref functions to fail.

<RTNCenteredText
  {...props}
  ref={element => {
    if (element) {
      Commands.trigger(element); // <-- trigger will not be called natively if batchRenderingUpdatesInEventLoop is turned ON
    }
  }}
/>

The corresponding NativeCommand:

// RTNCenteredText.mm
- (void)trigger {
    NSLog(@"*** Fabric component trigger method called directly");
}

- (void)handleCommand:(const NSString *)commandName args:(const NSArray *)args {
    NSString *TRIGGER = @"trigger";
    if([commandName isEqual:TRIGGER]) {
        [self trigger];
    }
}

Diagnosis

The NativeCommand trigger fails because when synchronouslyDispatchCommandOnUIThread gets called, findComponentViewWithTag returns nil because the _registry does not contain the element.

When batchRenderingUpdatesInEventLoop is off, the _registry is correctly populated with all elements on the screen, and the NativeCommand trigger functions correctly.

If Commands.trigger is wrapped in a setTimeout, it gets called successfully.

<RTNCenteredText
  {...props}
  ref={element => {
    if (element) {
      setTimeout(() => {
        Commands.trigger(element); // <-- trigger gets called successfully
      }, 0);
    }
  }}
/>

Steps to reproduce

See the reproducer provided.

  1. Use codegenNativeComponent and codegenNativeCommands to create a NativeCommand
  2. Call the created NativeCommand in a ref function
  3. See that the NativeCommand does NOT get called in versions >=0.74.1

React Native Version

0.74.5

Affected Platforms

Runtime - iOS

Areas

JSI - Javascript Interface, Bridgeless - The New Initialization Flow

Output of npx react-native info

N/A

Stacktrace or Logs

N/A

Reproducer

https://github.com/RyanCommits/RN74-issue-reproducer

Screenshots and Videos

No response

@RyanCommits RyanCommits added Needs: Triage 🔍 Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules) labels Sep 4, 2024
@react-native-bot
Copy link
Collaborator

⚠️ Newer Version of React Native is Available!
ℹ️ You are on a supported minor version, but it looks like there's a newer patch available - 0.74.5. Please upgrade to the highest patch for your minor or latest and verify if the issue persists (alternatively, create a new project and repro the issue in it). If it does not repro, please let us know so we can close out this issue. This helps us ensure we are looking at issues that still exist in the most recent releases.

@react-native-bot react-native-bot added the Platform: iOS iOS applications. label Sep 4, 2024
@react-native-bot
Copy link
Collaborator

⚠️ Newer Version of React Native is Available!
ℹ️ You are on a supported minor version, but it looks like there's a newer patch available - undefined. Please upgrade to the highest patch for your minor or latest and verify if the issue persists (alternatively, create a new project and repro the issue in it). If it does not repro, please let us know so we can close out this issue. This helps us ensure we are looking at issues that still exist in the most recent releases.

@RyanCommits
Copy link
Author

Upgraded reproducer to 0.74.5

@cortinico cortinico added Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. and removed Needs: Triage 🔍 labels Sep 12, 2024
@elencho
Copy link
Contributor

elencho commented Sep 17, 2024

@cortinico can I work on this?

@cortinico
Copy link
Contributor

@elencho feel free to investigate further.

Context from @rubennorte:

Clarification: this problem isn't specific to that flag, but some changes in timing can make it worse. That kind of code isn't guaranteed to work correctly with that flag disabled either.

Context: on Android, view mutations and view commands are dispatched in the same queue, so if you create a view (which happens before you get a ref) and you dispatch a command (which you do through a ref), those operations happen in order. On iOS, view mutations and view commands are processed separately (view mutations are queued but view commands are immediately dispatched to the host platform). This causes the request for the view to be queued and the view command to be dispatched immediately, which causes the command not to find the view in most cases (especially if we delay creation via batchRenderingUpdatesInEventLoop).

Solution: the solution for this is to queue view commands in the same queue where we queue view mutations on iOS.

@RyanCommits
Copy link
Author

Our library is dependent on the immediate firing of Commands in the component ref.

As we wait for this change, what would you @cortinico suggest as a workaround for now on this race condition? Is using requestAnimationFrame to wrap Commands a stable enough of a workaround for the view mutations to finish before the view commands fire?

@rubennorte
Copy link
Contributor

Our library is dependent on the immediate firing of Commands in the component ref.

As we wait for this change, what would you @cortinico suggest as a workaround for now on this race condition? Is using requestAnimationFrame to wrap Commands a stable enough of a workaround for the view mutations to finish before the view commands fire?

requestAnimationFrame is essentially setTimeout(callback, 0) at the moment, so it's not really helpful for this.

I'm not familiar with your use case, so it's hard to suggest a workaround. Maybe you can queue those commands in native instead?

@RyanCommits
Copy link
Author

At Fullstory we use method swizzling to get React classes to call our Commands.

All of this is to allow us to read a custom prop in the native code like:
<View customProp="customValue">

In new architecture, everything is strongly typed in C++, so this is our workaround to read customProp values off of a component for our native code to process.

We need to read these props on view creation, and since it's React Classes that we're calling Commands on, I think to leverage a queue, we'd have to rely on React Native's internal implementation.

@rubennorte
Copy link
Contributor

@RyanCommits oh, I see. Would it make sense to use custom components instead of the RN built-ins in that case? Maybe replace View with a custom implementation that handles those attributes properly?

@RyanCommits
Copy link
Author

The idea is for our users to be able to plug and play - install our plugin with minimal code changes and be able to tag their components. We could build a custom component, but then our users will have to replace every component that requires this feature. We would also lose the ability to use our babel plugin to auto-tag components with identifying props, including components within other libraries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. Newer Patch Available Platform: iOS iOS applications. Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules)
Projects
None yet
Development

No branches or pull requests

5 participants