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

How can I force update a class component in SyncLane? Can we permanently expose runWithPriority API? #28897

Closed
zhantx opened this issue Apr 24, 2024 · 13 comments
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@zhantx
Copy link

zhantx commented Apr 24, 2024

In react 18 concurrency, sometime we want to specify a higher priority for UI update triggered by external store.

This can be done for function component by using useSyncExternalStore.

However, I can't find any information on how to make class component's forceUpdate to be in SyncLane.

In react experimental version, I can use unstable_runWithPriority API to achieve this. Is there a more stable way to do it?

@zhantx zhantx added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Apr 24, 2024
@rickhanlonii
Copy link
Member

https://react.dev/reference/react-dom/flushSync

@zhantx
Copy link
Author

zhantx commented Apr 24, 2024

Hi @rickhanlonii , flushSync isn't very suitable in my case. I'm using mobx as the external store, it essentially calls forceUpdate when the store changes.
Say I have 2 class components observing 1 store, if I use flushSync(component.forceUpdate) on both components, it will cause react to perform 2 complete render cycles.
However, if I use runWithPriority(1, component.forceUpdate) on them, react will batch the 2 updates into 1 render cycle

@haskellcamargo
Copy link

Same problem here. flushSync is not working for class components when using one of the latest versions of MobX React as it's causing tearing. Functional components were fine after upgrading MobX React but class components are still causing weird UI bugs, making, for instance, different loading placeholders show twice. This only happens with the concurrent renderer though.

@rickhanlonii
Copy link
Member

runWithPriority(1, component.forceUpdate) is essentially the same as:

queueMicrotask(() => {
  flushSync(() => forceUpdate())
})

Can you see if that works?

The tearing is expected if mobx isn't using useSyncExternalStore and if it is, then the extra updates are expected due to the additional renders from the sync updates when the store changes. External stores are not supported in concurrent rendering.

@zhantx
Copy link
Author

zhantx commented Apr 24, 2024

@rickhanlonii mobx is using useSyncExternalStore, however, there is no equivalent API for class component, that's where the tearing happens
consider below code in react@18.2.0 and react-dom@18.2.0

import React, { useSyncExternalStore } from "react";
import { flushSync, runWithPriority } from "react-dom"; // I patched react-dom to expose runWithPriority

class Store {
  //  implementation of Store...
}
const store = new Store(0);

class DisplayValueClass extends React.Component {
  unsubscribe?: () => void;
  componentDidMount() {
    this.unsubscribe = store.subscribe(() => {
      // Option 1 call forceUpdate directly, this will be in DefaultLane
      // this.forceUpdate();

      // Option 2 wrap forceUpdate in flushSync
      // flushSync(() => this.forceUpdate());

      // Option 3 wrap forceUpdate in flushSync and queueMicrotask
      // queueMicrotask(() => {
      //   flushSync(() => this.forceUpdate());
      // });

      // Option 4 wrap forceUpdate in runWithPriority
      // runWithPriority(1, () => {
      //   this.forceUpdate()
      // });
    });
  }
  componentWillUnmount() {
    this.unsubscribe?.();
  }

  render() {
    return <div>Value: {store.getValue()}</div>;
  }
}

const DisplayValueFunction = () => {
  const storeValue = useSyncExternalStore(store.subscribe, store.getValue);
  return <div>Value: {storeValue}</div>;
};

const App = () => {
  return (
    <>
      <button
        onClick={() => {
          requestAnimationFrame(() => {
            // use requestAnimationFrame to make it outside of event handler, running in DefaultLane
            // this could be a setTimeout as well
            store.setValue(store.getValue() + 1);
          });
        }}
      >
        increment
      </button>
      <DisplayValueClass />
      <DisplayValueClass />
      <DisplayValueFunction />
    </>
  );
};

when the external store updates, because DisplayValueFunction uses useSyncExternalStore, the update is scheduled in SyncLane
For DisplayValueClass, it subscribe to the store in componentDidMount, and the subscriber is to force update itself, we have 4 options here (I attached the main thread snapshots below):

  1. call this.forceUpdate() directly. this will make the update scheduled in DefaultLane
  2. wrap forceUpdate in flushSync, because we have 2 DisplayValueClass instances, they will be in seperate render cycle
  3. wrap forceUpdate in flushSync and queueMicrotask, see screenshot 3, we still have multiple render phase
  4. wrap forceUpdate in unstable_runWithPriority, this is by far the best result

option 1 result
Body

option 2 result
Body (1)

option 3 result
Body (2)

option 4 result
Body (3)

Also, I didn't want to use queueMicrotask because it depends on it is identical to react internal logic for SyncLane, which could change without notifying us. Where if react can expose runWithPriority (which is essentially flushSync without flushPassiveEffects and flushSyncCallbacks) then I can use it with cautious

@zhantx
Copy link
Author

zhantx commented Apr 24, 2024

also, regarding option 3, queueMicrotas didn't just add 1 extra render, it adds 1 extra render per class component subscriber
image
image

@samcooke98
Copy link

Also... 🙈 - This method is currently exposed (admittedly with a todo) in the experimental channel; https://github.com/facebook/react/blob/main/packages/react-dom/index.experimental.js#L17

@zhantx
Copy link
Author

zhantx commented Apr 24, 2024

More consideration on why I prefer runWithPriority over queueMicrotask + flushSync
I could use one queueMicrotask + flushSync to batch several forceUpdate calls, however:

  1. queueMicrotask is a low level API in react internal compare to runWithPriority, I'd prefer a higher level API to expose
  2. flushSync reinstate the behaviour of legacy mode by calling flushPassiveEffects at beginning, where runWithPriority allows me to update class component in concurrent mode with specified Lane. Please correct me if I'm wrong, in my understanding the legacy mode is different from concurrent mode's SyncLane (e.g. in concurrent SyncLane, passiveEffects are flushed at end, and if a passive effect causes UI update, it will be in DefaultLane), which means if I use flushSync on class components, I will have a mixed behaviour of concurrent mode SyncLane and legacy mode

@rickhanlonii
Copy link
Member

This sounds like it's really about batching and not scheduling behavior. Does wrapping it in setTimeout work?

Note: we're removing runWithPriority in React 19.

@zhantx
Copy link
Author

zhantx commented Apr 24, 2024

I think eventually what I want to achieve is "schedule class component updates in SyncLane" and "function component and class component get re-rendered together"

If in a click event handler, it calls a setState for a class component, and calls setState (from useState hook) for a function component, then both class component and function component are scheduled in one microtask, and react renders them together

However, when it comes to external store, in a non-event handler, function component still scheduled in SyncLane, but the class component is scheduled in DefaultLane. even if I use queueMicrotask + flushSync, I can't schedule the class component to be in the same microtask as function component

@rickhanlonii
Copy link
Member

Ah right -can you testd in the react canary? In the canary the default updates and sync updates are flushed together so they're batched.

@zhantx
Copy link
Author

zhantx commented Apr 24, 2024

I tested react canary, it does batch default updates and sync updates, i.e. for above code example with option 1, that class component and function component are re-rendered together in a microtask, which is what I wanted to achieve. It would be really appreciated if you could link me the PR that introduces this found it, I think it's this one

However, if I remove function component, that only class components are subscribed to the store update, then class components are updated in a task not microtask. this brings more uncertainty to the behaviour

@zhantx
Copy link
Author

zhantx commented Apr 26, 2024

@rickhanlonii I just tested react 19 beta, and its behaviour is identical to what I saw in canary that default updates advance to microtask to be flushed with sync updates, however, if there is no sync updates (i.e. no useSyncExternalStore is used), then default updates stays in async task
This is an unreliable fix that doesn't guarantee when class component is updated. I still want to request the runWithPriority API to be reinstated and exposed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

4 participants