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

broken in react 18 #100

Closed
dontsave opened this issue Jan 27, 2023 · 7 comments
Closed

broken in react 18 #100

dontsave opened this issue Jan 27, 2023 · 7 comments

Comments

@dontsave
Copy link

dontsave commented Jan 27, 2023

hi there. I love use-context-selector but after recently upgrading a large codebase to react 18, we noticed that it stopped behaving as expected. subscribers to the context are rerendering, regardless of whether their selector returns a changed value or not. for example in this case:

import { memo, useState } from 'react';
import { createContext, useContextSelector } from 'use-context-selector';

export default {
  title: 'spike',
  component: null,
};

export const Demo = () => {
  const [state, setState] = useState({ count: 0, foo: false });
  return (
    <CTX.Provider value={state}>
      <div>
        <button onClick={() => setState((prev) => ({ ...prev, foo: true }))}>
          button
        </button>
        <div>
          {[...new Array(100)].map((_, i) => {
            return <Div key={i} id={i} />;
          })}
        </div>
      </div>
    </CTX.Provider>
  );
};

const CTX = createContext({ count: 0 });

const Div = memo(({ id }: any) => {
  const count = useContextSelector(CTX, (val) => val.count); // <-- always 0!!
  console.log(`!`); // <-- logs 100 times!

  return <div key={id}>{count}</div>;
});

clicking the button changes a part of the state that is not used by any of the consumers of the context. however they all still rerender anyway. this seems to defeat the purpose of the library, which is to prevent unnecessary rerenders. any guidance you have would be appreciated here

@dai-shi
Copy link
Owner

dai-shi commented Jan 28, 2023

If you put the console.log statement in useEffect, it shows just once, right?

If that's the case, it's expected behavior, React 18 disables useReducer early bailout and it's how it works.

If this behavior causes a performance issue in your app, the solution is to use useMemo to avoid repeating computation.

@dai-shi
Copy link
Owner

dai-shi commented Jan 29, 2023

I remember something. If you have a stable selector, it can be optimized.
Please try defining the selector outside component, or use useCallback.

@dai-shi
Copy link
Owner

dai-shi commented Jan 29, 2023

I remember something. If you have a stable selector, it can be optimized. Please try defining the selector outside component, or use useCallback.

oops, my bad. optimization with stable selector is about calling the selector. so, it doesn't apply in this case.

@dontsave
Copy link
Author

dontsave commented Jan 31, 2023

@dai-shi thanks for the quick response. You're right, a useEffect only fires once in the consumer of the context, on first mount. And in a further test, children of the consumer do not rerender at all, even when unmemoized and passed random numbers as props! I take it this is due to the "rerender without commit" behavior from the useReducer case you tweeted about? It's very counterintuitive, and I'm not sure I fully understand it.

The behavior brings to mind two worries: the first is that it could confuse engineers who encounter an unexpected "rerender" and spend time trying to track it down, and the second is a perf concern with more computation now done inline in the function body. So for example a simple unmemoized inline loop (say Array.filter) performed by a single consumer wouldn't be that bad, but now if hundreds of consumers run that simple inline loop, we have a potential performance hiccup. I saw in the other thread you mention this behavior is required to support concurrent mode without tearing and I am wondering if there may be a way to allow that support be opt-in (or opt-out?) Or would that undermine the core intentions of this library?

Regardless I appreciate the explanation and the work done here. For the time being I think we will monitor performance and apply memoization where necessary.

@dai-shi
Copy link
Owner

dai-shi commented Feb 1, 2023

(I feel like I answered to a similar question somewhere. pmndrs/valtio#638 (reply in thread) is it...)

The first concern is valid, but people misunderstand how react optimizes renders. I would suggest, if they want to debug extra re-renders with console.log, to use useEffect.
For the second concern, useMemo is the right solution.

Or would that undermine the core intentions of this library?

Correct. The motivation of this library is to make it compatible (as much as possible in userland) with concurrency. I run experiment here: https://github.com/dai-shi/will-this-react-global-state-work-in-concurrent-rendering It's kind of opt-in, if you use useContextUpdate, it renders more.

btw, with React 17, it works as expected, right?

If you want something for legacy react (no concurrency), creating use-context-selector API compatible library is fairly easy (using useRef hacks. Mostly, it's like react-redux v7.)

Especially, if we require stable selectors (always use useCallback), it can be implemented without useRef.

There are such libraries out there, but if you are interested, I can show the rough idea.

@dontsave
Copy link
Author

dontsave commented Feb 1, 2023

yes the behavior was as expected in 17. in 18 we'll continue to monitor our app and memoize as necessary. while we haven't adopted much concurrency yet, switching back to a legacy library which could introduce potential tearing may be something we regret later. thanks for the response

@dai-shi
Copy link
Owner

dai-shi commented Feb 1, 2023

In retrospect, this library is developed for concurrency (it's a spin-off from react-tracked), and in v1.3, it moved to a new implementation that requires useContextUpdate. It requires extra renders (no commits) to pass my tests, even in react 17. Using useContextUpdate was default in react-tracked, but meanwhile people got confused with extra-renders-without-commits behavior. So, I made it opt-in because there's no point of supporting concurrency in react 17. React 18 changes useReducer behavior, and now use-context-selector requires extra-renders-without-commits, even without useContextUpdate. The useReducer behavior change is to solve some edge cases, maybe that includes concurrent use cases. There might be room for improvement in the current use-context-selector, if we target only react 18. My gut feeling is it will still use useReducer.

With that, let's close this.


On side note: The goal of this library is not performance. If people are trying to use this library for global state solution, and they care performance, I'd recommend trying global state libraries, which could be more performant. Here's some of my projects: jotai, valtio, and zustand.

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

No branches or pull requests

2 participants