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

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

Open
yamadapc opened this issue Mar 5, 2024 · 5 comments
Open
Labels
duplicate This issue or pull request already exists

Comments

@yamadapc
Copy link

yamadapc commented Mar 5, 2024

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
Copy link
Collaborator

albertogasparin commented Mar 5, 2024

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

@albertogasparin albertogasparin added the duplicate This issue or pull request already exists label Mar 5, 2024
@yamadapc
Copy link
Author

yamadapc commented Mar 5, 2024

Is it duplicated?

@yamadapc
Copy link
Author

yamadapc commented Mar 5, 2024

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
Copy link
Collaborator

albertogasparin commented Mar 5, 2024

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
Copy link
Collaborator

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

defaults.batchUpdates = true

which is what we use across some products in production

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

2 participants