atlassian / react-sweet-state

Shared state management solution for React
https://atlassian.github.io/react-sweet-state/
MIT License
871 stars 55 forks source link

Using container on update hook causes React 16 errors to be printed #223

Open yamadapc opened 8 months ago

yamadapc commented 8 months ago

When using container onUpdate hooks and setting state in them, react-sweet-state may call set state during render when the container props change.

This causes a warning on react 16 "Warning: Cannot update a component (Child) while rendering a different component".

The problem is caused by this section running on render:

    if (!shallowEqual(propsRef.current.next, propsRef.current.prev)) {
      containedStores.forEach(({ handlers }) => {
        handlers.onContainerUpdate?.(
          propsRef.current.next,
          propsRef.current.prev
        );
      });
    }

The following example reproduces the bug:

import "./styles.css";
import React, { useState } from "react";
import { createHook, createContainer, createStore } from "react-sweet-state";

export const hydrateContainer =
  () =>
  ({ setState }, { value }) => {
    setState({
      value,
    });
  };

const Store = createStore({
  actions: {},
  initialState: { value: 0 },
});
const Container = createContainer(Store, {
  onInit: hydrateContainer,
  onUpdate: hydrateContainer,
});

const useStore = createHook(Store);

function Child() {
  const [{ value: storeValue }] = useStore();
  return <div>storeValue: {storeValue}</div>;
}

export default function App() {
  const [value, setValue] = useState(0);

  return (
    <Container value={value}>
      value: {value}
      <Child />
      <button onClick={() => setValue((v) => v + 1)}>
        Click to bug out sweet state!
      </button>
    </Container>
  );
}

A sandbox reproduction is on - https://codesandbox.io/p/sandbox/cranky-dew-lvdhwv?file=%2Fsrc%2FApp.jsx%3A1%2C1-42%2C1

albertogasparin commented 8 months ago

Duplicated #219 We have a PR up #221 that however needs validation

yamadapc commented 8 months ago

Is it duplicated?

yamadapc commented 8 months ago

This isn't really a "change in behaviour caused by React 17 to 18", but it is a bug in React 16 where using the setState API in onUpdate will always trigger an error to be logged.

This is a problem because developers will be confused by the warnings being logged and not know that they are coming from React Sweet State.

This is still an issue with certain options after #221.

I suggest the setState parameter be removed from the update hook API on the cases it will break / print a warning.

albertogasparin commented 8 months ago

Marked as duplicated because it has the same root cause: the conversion to functional component. By going back to class and using getDerivedStateFromProps to trigger those state changes, the warning and the deoptimisation go away (could not reproduce anymore). I assume because that static method is called before render phase and so mutating state is still a valid operation

albertogasparin commented 8 months ago

While we work on #221 , a workaround is to set

defaults.batchUpdates = true

which is what we use across some products in production