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

useEffect hooks do not seem to synchronize with XAML's render pass #9505

Closed
chswan opened this issue Feb 8, 2022 · 11 comments
Closed

useEffect hooks do not seem to synchronize with XAML's render pass #9505

chswan opened this issue Feb 8, 2022 · 11 comments
Labels
Area: View Managers enhancement Old Architecture Broad category for issues that apply to the RN "old" architecture of Cxx Modules + Paper Partner: Microsoft Recommend: Not Planned Recommend that issue should be given Not Planned milestone.
Milestone

Comments

@chswan
Copy link

chswan commented Feb 8, 2022

Problem Description

The React documentation for useEffect states:

The function passed to useEffect will run after the render is committed to the screen

In my observation this does not seem to be the case today. Specifically, it looks like at the moment useEffect fires, the XAML tree has been updated but XAML has not itself run a render pass against its tree.

This is impactful because it makes it difficult to show a new top-level window containing RNW content without flickering -- I can't determine "OK, now is a good time to call ShowWindow(SW_SHOW)".

Granted: I believe that XAML doesn't currently expose its render state in a way that would allow this to be implemented to spec. I'm hoping this project can help push to prioritize any prerequisite functionality from XAML.

Steps To Reproduce

Note: I was reproducing this in as close to a release configuration as possible, loading a Hermes bundle from disk, with everything possible built in Release mode. I reproduced this myself with a kernel debugger to catch the DebugBreak because that's what I had handy. There are probably other ways to do this like setting a breakpoint in an existing module implementation that you likewise call from useEffect() rather than a custom module with an embedded DebugBreak, I just went with what was easy.

  1. Set up a project with a simple native module with a function called something like breakIntoDebugger(), whose implementation calls DebugBreak().
  2. Set up a test app that creates a ReactRootView on-demand in response to a XAML button click (so that you can clearly see the behavior after the app's window is onscreen) and loads a simple React component
  3. In the React component, return some simple Hello World UI.
  4. In the React component, register a useEffect() hook and call the breakIntoDebugger() native module function inside of it.
  5. Run the app, attach a native debugger to the process, and click the button. Observe the state of the UI when the DebugBreak hits.

Expected Results

Expected: the Hello World content is on screen

Actual: the Hello World content is not yet on screen. I sometimes see blank UI and sometimes see the "Bundle Loading" UI. However, analyzing the state of the XAML tree reveals that the Hello World content is there.

CLI version

6.3.1

Environment

Sorry, this command doesn't work for me (cannot resolve "wmic")

Target Platform Version

No response

Target Device(s)

Desktop

Visual Studio Version

No response

Build Configuration

Release

Snack, code example, screenshot, or link to a repository

Sorry, I do not have a publicly shareable minimal repro right now. MSFT people, please reach out to me and I can share what I have.

@chswan chswan added the bug label Feb 8, 2022
@ghost ghost added the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Feb 8, 2022
@asklar
Copy link
Member

asklar commented Feb 9, 2022

@acoates-ms fyi; does this sound familiar?

@chrisglein
Copy link
Member

Actual: the Hello World content is not yet on screen. I sometimes see blank UI and sometimes see the "Bundle Loading" UI. However, analyzing the state of the XAML tree reveals that the Hello World content is there.

This sounds to me like the batch of commands hasn't been flushed to the compositor yet. XAML's state in memory will reflect what will be there once that happens, but if the expectation of useEffect is after bits hit the screen then it needs to be dispatched after the flush.

Environment: Sorry, this command doesn't work for me (cannot resolve "wmic")

@asklar This sounds familiar but I couldn't find the issue. Is there a known issue with the dependency script in this configuration?

@chrisglein chrisglein added this to the 0.69 milestone Feb 10, 2022
@asklar
Copy link
Member

asklar commented Feb 10, 2022

@chrisglein re: wmic not working - this is not an issue with the dependencies script but with the RN core CLI - react-native-community/cli#1513 arising from the removal of WMIC from Windows 11 onwards.

@NickGerleman
Copy link
Collaborator

useEffect does not, on any platform, synchronize with native render. It instead is called when the component is mounted into the React Tree, which happens much earlier.

@chswan
Copy link
Author

chswan commented Feb 10, 2022

@NickGerleman thank you for that clarification. In addition to the doc snippet where it mentions "committed to the screen", later it calls out that it fires "after the browser has painted". Should that be considered a doc bug on the ReactJS side I should follow up on?

@NickGerleman
Copy link
Collaborator

NickGerleman commented Feb 13, 2022

It gets a little messy. A lot of the public documentation for react targets react-dom (Web React) instead of react-native. I think on the browser, it is true, since it is single-threaded. In React Native, there are separate JS and UI threads, that have different lifecycles and responsibilities.

In react-native, useEffect is first fired when the react component tree has been committed for the frame. This "commit" happens by handing the work from the JS thread to the UI thread as "shadow nodes". useEffect is by default called on every subsequent react render as well, but you can pass a list of dependencies that should cause a re-render. There is a "Rules of Hooks" ESLint plugin you're likely already using, which includes react-hooks/exhaustive-deps to make this easier.

There's a prop on native components, onLayout, that delivers a message to JS with dimensions for the component. This is still allowed to happen before the tree is complete, rendered, or presented in native.

There's some good discussion on the issue of synchronizing JS controlled behavior here. #9292 . Notably a lot of what I said is something that will change in the future, with Fabric, React Native's new UI architecture.

@NickGerleman
Copy link
Collaborator

This is a great resource for Fabric: https://reactnative.dev/docs/render-pipeline

@chrisglein
Copy link
Member

Two things here:

  • Are we meeting the expectations for when useEffect should fire?
  • For this specific measuring case is everyone unblocked to use the native XAML events to get what they need?

To our understanding, RNW's useEffect is matching expectations relative to the other platforms. So we should focus on whether it's possible to hook the right native events to know when the right batch is flushed. Which requires knowing that the UI has been created, and then the batch is complete. A strategy that's been used before for this is to write a dummy view manager that doesn't do anything other than hook the native events at what should be the timing for the others.

@chrisglein chrisglein added enhancement Area: View Managers and removed bug Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) labels Feb 14, 2022
@chrisglein
Copy link
Member

For this issue, I think we're clear on whether we want to change useEffect, which means the work we want to track is whether @chswan can get their needs met with the dummy view manager strategy. If so, let's close this and track a new issue for building that out. Sound good @chswan?

@jonthysell
Copy link
Contributor

@asklar Is this still something that makes sense for 0.69 or should it be moved to 0.70? Or maybe the Backlog?

@asklar
Copy link
Member

asklar commented May 6, 2022

backlog seems ok; @chswan any objections?

@asklar asklar modified the milestones: 0.69, Backlog May 24, 2022
@asklar asklar removed their assignment May 24, 2022
@jonthysell jonthysell added the Old Architecture Broad category for issues that apply to the RN "old" architecture of Cxx Modules + Paper label Mar 8, 2024
@chiaramooney chiaramooney added the Recommend: Not Planned Recommend that issue should be given Not Planned milestone. label Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: View Managers enhancement Old Architecture Broad category for issues that apply to the RN "old" architecture of Cxx Modules + Paper Partner: Microsoft Recommend: Not Planned Recommend that issue should be given Not Planned milestone.
Projects
None yet
Development

No branches or pull requests

7 participants