facebookexperimental / Recoil

Recoil is an experimental state management library for React apps. It provides several capabilities that are difficult to achieve with React alone, while being compatible with the newest features of React.
https://recoiljs.org/
MIT License
19.59k stars 1.18k forks source link

Warning: Cannot update a component (`xxx`) while rendering a different component (`xxx`). #12

Open syunto07ka opened 4 years ago

syunto07ka commented 4 years ago

A warning is occured where useRecoilValue() is used even though I write a code like one which is in the official document here.

スクリーンショット 2020-05-15 4 55 26

this is my code

import React, { useEffect } from 'react';
import { atom, useRecoilState, useRecoilValue, selector } from 'recoil';

function RecoilApp() {
  useEffect(() => {
      console.log('test');
  });
  return(
    <div>
      Recoil App
      <TextInput />
      <CharCount />
    </div>
  );
}

const textState = atom({key: 'textState', default: ''});

function TextInput() {
  const [text, setText] = useRecoilState(textState);

  const onChange = event => {
    setText(event.target.value);
  }

  return(
    <div>
      <input type="text" value={text} onChange={onChange} />
      <br />
      Echo: {text}
    </div>
  );
};

function CharCount() {
  const count = useRecoilValue(countState);
  return <>Charset Count: {count}</>;
}

const countState = selector({
  key: 'countState',
  get: ({get}) => {
    const text = get(textState);
    return text.length;
  },
});

export default RecoilApp;
ecreeth commented 4 years ago

I have copied and pasted your code and it works. See example in codesandbox

syunto07ka commented 4 years ago

Thanks, ecreeth. As you say that, my code works, but warning is occured in my browser.

Humm, I wonder that the codesandbox occured no warning at console like the image.

dai-shi commented 4 years ago

Humm, I wonder that the codesandbox occured no warning at console like the image.

You need to try with react v16.13.1. https://codesandbox.io/s/async-pine-gogdb

(edit: I'm surprised to get so many down votes. Many people probably misunderstood my comment is how to fix the warning. My apologies for not clarifying the context. It was a comment for @syunto07ka to "reproduce" the warning in codesandbox.)

slapner commented 4 years ago
Screen Shot 2020-05-15 at 9 19 01 AM

The error is present in both the codesandbox links

syunto07ka commented 4 years ago

it's my installed dependences in pakcage.json. so yeah, "16.13.1" is correct.

"dependencies": {
    "@testing-library/jest-dom": "^4.2.4",
    "@testing-library/react": "^9.5.0",
    "@testing-library/user-event": "^7.2.1",
    "react": "^16.13.1",
    "react-dom": "^16.13.1",
    "react-scripts": "3.4.1",
    "recoil": "0.0.7"
  },
davidmccabe commented 4 years ago

Thanks for reporting, I am investigating this.

somonek commented 4 years ago

Maybe something related to hot-loader? I just had:

Warning: Cannot update a component from inside the function body of a different component.

then I removed the react-dom alias from my webpack config

alias: {
   'react-dom': '@hot-loader/react-dom',
},

and the error changes to

Warning: Cannot update a component (`Batcher`) while rendering a different component (`Header`). To locate the bad setState() call inside `Header`, follow the stack trace as described in https://fb.me/setstate-in-render

Header is my component name...

djwglpuppy commented 4 years ago

Maybe something related to hot-loader? I just had:

Warning: Cannot update a component from inside the function body of a different component.

then I removed the react-dom alias from my webpack config

alias: {
   'react-dom': '@hot-loader/react-dom',
},

and the error changes to

Warning: Cannot update a component (`Batcher`) while rendering a different component (`Header`). To locate the bad setState() call inside `Header`, follow the stack trace as described in https://fb.me/setstate-in-render

Header is my component name...

I noticed also that setters stop working when hot reloading is set ... so having this related as well sounds like something worth exploring

drarmstr commented 4 years ago

Yes, we have a known issue with hot module loading at the moment.

vandercloak commented 4 years ago

Having the same issue. Thanks for reporting @syunto07ka and thanks for investigating @davidmccabe.

lindskogen commented 4 years ago

I have this issue and I'm not using react hot module loading.

incepter commented 4 years ago

The Batcher's component's setState is the state setter that is being called, and the main cause is because replaceState from RecoilRoot is called when rendering subscribed components.

The warning was initially added to react on the v16.13.0 release, and highlights an anti-pattern of the react mental model.

Inlining the logic in the batcher's effect to replaceState solves the issue (along with setTimeout on the notifyBatcherOfChange.current(), but I don't believe that setTimeout is a reliable solution). While RecoilRoot wraps a context provider, a state inside it could be a solution, but we will return to the initial problem.

I understand that the main goal of the Batcher is to catch as many setState calls from subscribers as possible and delegate to react batching to gather queuedComponentCallbacks and optimize the count of re-renders. Moreover, the subscribed component callback is the forceUpdate (state setter) of useInterface that is linked to subscribed components. So if many updates occur in a short duration, I prefer leaving react to handle the batching stuff rather than building a logic on top of it, and also because it is unstable.

Besides, the replaceState will notify the batcher only with the new state reference change, and as far as I read, this is the case when a new subscription occurs.

My question is, Shouldn't we drop the batcher and leave react manage the batching of state updates ? I mean, it is like an abstraction over an abstraction, or am I mistaking something?

I think to solve the problem without dropping the Batcher, a work loop that schedules when to flush the subscriptions is mandatory (may have its own problems though).

jjrdn commented 4 years ago

What if Batcher itself was rendered whenever a selector was rendered. eg, treat it as if it depends on all selectors. Then it would automatically queue its useEffect, and you would not need notify the Batcher of a change in replaceState, Batcher could test for the change when running it's useEffect. I think this also solves the "double render" whenever a selector changes dependencies.

jjrdn commented 4 years ago

I see now that replaceState is called under other conditions that are not part of a render.

This particular issue occurs because:

  1. selectors are run during render
  2. selectors update their dependencies when they run
  3. updating dependencies calls replaceState
  4. replaceState notifies the Batcher through use of setState on the Batcher, which happens during the render of the component that uses the selector
davidmccabe commented 4 years ago

I'm working on a big refactor where dependencies will not go through React state at all. That will fix this (and make it a lot more efficient, and is needed for CM). In the meantime I don't see a an obvious workaround unfortunately.

atanasster commented 4 years ago

@davidmccabe - that would be great - pls post any updates when you have an ETA. Thanks again.

avj2352 commented 4 years ago

I am getting the same error, In my case this error pops up if I use the selector variable as a dependency in my component's useEffect For eg: in recoil:

// get decoded URL of meetingId
export const getDecodedMeetingId = selector ({
    key: 'decodedMeetingIdState',
    get: ({get}: any) => {
        const meetingId = get(meetingIdState);
        return decodeURI(meetingId);
    }
});

And then my component useEffect

const meetingStatus = useRecoilValue(getDecodedMeetingId);
// side-effects
    useEffect(()=>{
        if (meetingStatus) {
            console.log('Begin Timer');
            startTimer();
        } else {
            setTimer(prev => prev);
        }
    },[meetingStatus]);

Hopefully this helps. If I am doing something wrong, please let me know.

markerikson commented 4 years ago

@davidmccabe I'm very curious - can you give any technical details on what you're doing and how it will improve CM compat? My general understanding was that any non-React-based state management approach will have issues with CM, because it can't tie in to React's ability to reorder state updates based on render priorities.

dai-shi commented 4 years ago

@markerikson That was my misunderstanding too, because they said at some point about considering the use of useMutableSource as one option.

https://github.com/facebookexperimental/Recoil/issues/5#issuecomment-628796942

In Recoil, state seems React based.

avj2352 commented 4 years ago

Hi, any ETA on this fix ? https://github.com/facebookexperimental/Recoil/issues/12#issuecomment-631201906 This is the problem, I am facing when using recoil... https://github.com/facebookexperimental/Recoil/issues/12#issuecomment-631238293

jaredpalmer commented 4 years ago

Confirming Recoil breaking hmr/fast refresh.

farukg commented 4 years ago

Hi, i found this comment from react-final-form where they had a similar issue and solved it: https://github.com/final-form/react-final-form/issues/751#issuecomment-606212893

Hope this helps

mphung97 commented 4 years ago

I have the same issue

markstock7 commented 4 years ago

I think replaceState shouldn't cause Batcher update when we call getRecoilValueAsLoadable, since getRecoilValueAsLoadable just get value from RecoilNode for computing derived value. I think the following code would work, But I am not for sure. @davidmccabe

const replaceState = (replacer, shouldNotNotifyBatcher) => {
    \\ other code
    if (shouldNotifyBatcher === true) return;
    nullthrows(notifyBatcherOfChange.current)();
};
function getRecoilValueAsLoadable<T>(
  store: Store,
  {key}: AbstractRecoilValue<T>,
): Loadable<T> {
  let result: Loadable<T>;
  // Save any state changes during read, such as validating atoms,
  // updated selector subscriptions/dependencies, &c.
  Tracing.trace('get RecoilValue', key, () =>
    store.replaceState(
      Tracing.wrap(state => {
        const [newState, loadable] = getNodeLoadable(store, state, key);
        result = loadable;
        return newState;
      }),
      true
    ),
  );
  return (result: any); // flowlint-line unclear-type:off
}
yishuxin commented 4 years ago

Same issue.

n8sabes commented 4 years ago

Hi @davidmccabe, Any ETA on this?

Inclusion of any selector in any FC whereby the selector calls get on an atom, generates this error. The simplest use case in my project is:

//
// foobarState.ts
//
export const foo = atom<boolean>({ key: "foo", default: true });
export const bar = selector<boolean>({ key: "bar", get: ({ get }) => !get(foo) });
//
// component.tsx
//
...
export default ((props) => {
    const foobar = useRecoilValue(bar);
    return <>Hello World</>
}

As an early adopter with high hopes for this project, I'm migrating away from Redux to Recoil!! Crossing fingers to see Recoil "stable enough" for our beta in a few months.

Thanks for your efforts on this!!

Tatametheus commented 4 years ago

I got his warning because I used a selector get another async selector get. Some example code

export const asyncSelector = selector>({
    key: 'AsyncSelector ',
    get: async ({ get }) => {
        const response = await fakeData();
        const data = await response.json();
        await new Promise((resolve) => {
            setTimeout(() => {
                resolve();
            }, 200);
        });
      return data
    },
});

export const getAsyncSelector= selectorFamily({
    key: 'GetAsyncSelector',
    get: (params) => ({ get }) => {
       return get(asyncSelector ).slice(params);
    },
});
nikitaeverywhere commented 4 years ago

Also encountered this warning today, decided to step back to react@16.12.0 and react-dom@16.12.0 in the meantime. Thanks guys for your work, this library looks very promising!

khanh19934 commented 4 years ago

Any update for this issue :( i try to add this to new project instead of redux ...

Holybasil commented 4 years ago
// store.js
export const todoListTypeState = atom({
  key: "todoListTypeState",
  default: "snack",
});

export const todoListState = atom({
  key: "todoListState",
  default: [
    { type: "snack", content: "cake" },
    { type: "drink", content: "water" },
  ],
});

export const todoListSelector = selector({
  key: "todoListSelector",
  get: ({ get }) => {
    const type = get(todoListTypeState);
    const todoList = get(todoListState);
    return todoList.filter((item) => item.type === type);
  },
  set: ({ set }, newValue) => {
    set(todoListState, newValue);
  },
});

// Todo.js
const todoList = useRecoilValue(todoListState); //  :} good
const [typedTodoList, setTodoList] = useRecoilState(todoListSelector); //  :{ warning

waitting...

andrecrts commented 4 years ago

same issue here. Thanks for your work.

MaxmaxmaximusGitHub commented 4 years ago

I found such a workaround, in child component i change parent component state with useEffect:

function Child () {
  const [ref, dimensions] = useDimensions()
  const {setDimensions} = useContext(ParentContext)

  // bad
  setDimensions(dimensions)

  // good
  useEffect(() => {
    setDimensions(dimensions)
  })

  return <div ref={ref}></div>
eLeontev commented 4 years ago

Looks like for now we need to live (and can) with such warning till Recoil team will provide refactored version of Batcher.

For now the warning is valid and based on adding new validation-warning from React team to highligh anti-pattern to use useState setter out of component which happens in this place. image

Downgrading React version to 16.12 will remove this issue if it annoying.

New warning React 16.13: https://ru.reactjs.org/blog/2020/02/26/react-v16.13.0.html#warnings-for-some-updates-during-render The details: https://github.com/facebook/react/issues/18178#issuecomment-602327720.

imho: any hacks and workaround will lead to complexity increasing and unexpected issues and it looks like a waste of time since the goal of this warning is only notify about the issue. And the fix should be centralized at least in this case.

inlet commented 4 years ago

Yep, same issue here.. it looks like the warning is only shown using selectors, pure atoms seems to be working without a warning.

Looking forward to a fix, thanks for all the hard work!

arakir commented 4 years ago

I have the same issue when an atom has a Promise as a default value.

natew commented 4 years ago

Should we track the HMR issue separately? For us that is the bigger blocker. Specifically, we see all state stop updating after any HMR.

drarmstr commented 4 years ago

@natew - there is #247 for HMR

thien-do commented 4 years ago

@drarmstr look like https://github.com/facebookexperimental/Recoil/pull/463 should fix this?

DaniCastel commented 4 years ago

I found this error when:

vivek12345 commented 4 years ago

do we have a fix for this? I am facing a similar error. Using recoil selector for an async call.

drarmstr commented 4 years ago

Also this commit and #399 help. These should limit to situations we see the warning, but it can still show up and we need to resolve that with React.

mehul-terminaltrend commented 4 years ago

I have copied and pasted your code and it works. See example in codesandbox

did you tried in CRA or custome react App?

shabanbx commented 4 years ago

Hi any fix on this issue?

ghost commented 4 years ago

Update: was using the example on the recoiljs website, changing it to SSR and works without problems, except a typescript error that isn't related to this issue.

In NextJS i'm seeing the error Cannot update a component (Batcher) while rendering a different component (xxx) only in development e.g. HMR - haven't seen it yet on after built.

el-angel commented 4 years ago

@nikolasveneti because production build does not log warnings

mattmogford-alcumus commented 4 years ago

Surprised there's been no updates on Recoil since June. I've just started using it and it is almost life changing. This pescy warning is annoying and hoping it will be fixed soon?

thien-do commented 4 years ago

@mattmogford-alcumus I completely agree that Recoil is life changing :D However it's not fair to say there's been no updates. There are quite frequent updates to master and I believe the team will eventually get to fix this issue :D There are several related commits already

mattmogford-alcumus commented 4 years ago

@mattmogford-alcumus I completely agree that Recoil is life changing :D However it's not fair to say there's been no updates. There are quite frequent updates to master and I believe the team will eventually get to fix this issue :D There are several related commits already

I just saw the comment from @davidmccabe talking about the re-work and subsequent fix. So that is great news! GitHub should have a way for Contributors to 'pin' an important comment such as that to the top, sometimes there is a lot of chaff to wade through to find the important information. I did mean updates as in a tagged version, but yes that was unfair to say with all the work on issues etc... aaand I'm adding to the chaff!

hayyaun commented 4 years ago

This also happened for me in a case in which a component was rerendering too fast.

Qquanwei commented 4 years ago

This happen because my component use useRecoilValue(selector)