What should we do with the warning "Cannot update a component from inside the function body of a different component." #7
Comments
|
Moving this to ‘useEffect’ might possibly circumvent this and make it more concurrent-friendly. This will have a small perf tradeoff I think but this could maybe be caught by wrapping in batchUpdates. |
|
Interestingly, I didn't think about useEffect... I thought there would be a tearing issue with useEffect, but not for this case because we have only one useEffect in the parent. I did use useLayoutEffect before obviously, which works perfectly but it de-opts to sync mode and we don't get benefit from concurrent mode. I tried https://github.com/dai-shi/will-this-react-global-state-work-in-concurrent-mode checking again. In summary, here's what I understand so far: in render: all checks pass check4 is also failing with use-subscription, so I might be doing something wrong. It would be so nice if we could use useEffect. Either, the check4 is wrong or there is another workaround. @JoviDeCroock Thanks for the hint! and questions are welcome. |
|
Hmm, I’ll have to double-check for myself. The only other wat I’m aware of would be a custom event emitter. Much like a proxy but a bit more manual book keeping, I’m making a PoC off that since that would increase perf even further by calling every entry max. once. That being said it should be concurrent since effects registered in P1 should be disposed on interrupt. I’ll check what the failing test is when I can open my laptop. EDIT: I did this, https://github.com/JoviDeCroock/hooked-form/pull/61/files#diff-b863cdcf29e9f6f006d596a9ac293ed4 seems to work well in concurrent too perf seems to improve too since we can't hit a callback twice anymore |
|
Hi @JoviDeCroock My understanding is that the use of useEffect could lead to some kind of inconsistency (tearing) because it's a passive effect (other code run before that).
I'm not sure if I understand this part. |
|
Hi, trying to solve a similar issue I came across this lib. I thought it's a good idea but I honestly do hate those 3 lines of code xD Here is a quick idea: const listeners = new Set()
const context = React.createContext(defaultValue, (prev, next) => {
if (prev === next || !listeners.size) return 0
listeners.forEach(l => l(next))
return 0
})Now, using |
|
@eddyw Hey, thanks for coming! Do you "hate" them because they are in render? I'm aware of useSubscription to some extent. It's failing my "check4" too. If it's reasonable, it's easy and we can simply move the three lines into useEffect. |
|
@dai-shi hm, yeah, it just doesn't feel "right" doing so in render. For instance, if the provider is a child of a container that manages its own local state, every render will also trigger calling all listeners on the provider which may be unnecessary. const context = React.createContext({ value, listeners }, (prev, next) => {
if (!next.listeners.length) return 0
if (prev === next && prev.listeners === next.listeners) return 0
next.listeners.forEach(l => l(next))
return 0
})A few points:
listeners = [ a, b, c ]
...
store.subscribe(() => { // <<< listener 'a'
removeListener(b)
addListener(d)
// we end up with: listeners = [a, c, d]
// while listener 'a' has been called
// next listener called should be .. ???
})What Redux does now is Finally, the order in which listeners are called is somewhat important. React Redux does create a kind of tree of listeners every time connect is used to ensure a parent component is updated before its children. It'd look kind of like (speudo-code, assuming all this components are connected to a store): [
listener-a, // <connected(Todo.Header)>
listener-b, // <connected(Todo)>
listener-b.listeners [ // connected children
listener-c, // <connected(TodoItem) id=1 >
listener-d, // <connected(TodoItem) id=1 >
]
]However, this is not possible to do with hooks, that's why react-redux warns about it if you opt to use hooks instead of At least these are the issues I'm having with my implementation. I hope this helps .. somehow xD |
|
@eddyw Hi, thanks for your detailed explanation.
True. Both calculateChangedBits and observedBits are unstable. Using observedBits in useContext raises a warning and I needed an undesired workaround in another lib of mine. Luckily, we don't have any warning for calculateChangedBits yet.
Yeah, but the rule can be changed. As we've been seeing this and the useContext warning.
You know what. This is very interesting to me. I haven't thought about passing listeners in the context value. This seems to do the hack.
Yeah, but again I'm not sure how reliable this is in the future.
I'm not sure if I understand this part. We never remove listeners in the callback.
I suppose this is incorrect. React Redux now uses
So, what are you trying in your implementation? |
Yet, I had thought about passing |
|
@dai-shi it doesn't have anything to do with batching updates and more with stale props and zombie children explained in react-redux while using hooks:
https://github.com/reduxjs/react-redux/blob/master/src/components/connectAdvanced.js#L394-L404 This is to ensure that listeners are called in a specific order, first the ones at the top, then the once nested inside a connected child component. However, as they mention in the docs, this seems not to be possible using just
It's written around
This is probably off-topic 🙈 |
|
OK, if you have Redux background. You might be interested in this thread.
My understanding is that stale props issue with hooks can't be solved in userland. (I wrote it in README).
You may or may not be interested in my other projects: this and that.
That's interesting. I actually meant React devtools, but still in that case it will show state but maybe not context. |
|
@dai-shi So trying to solve a completely different problem, I thought about this issue. Technically, there is an official way to expose instance methods to a parent using refs with This is kind of the idea: https://codesandbox.io/s/dazzling-euler-0kbce const ref = React.useRef();
const subscription = React.useRef();
...
React.useImperativeHandle(ref, () => ({
f: selector,
v: value,
s: selected
}));
React.useImperativeHandle(subscription, () => listener, [listener]);
React.useEffect(() => () => listeners.delete(subscription), [listeners]); // <<< handle unsubscribe
if (!listeners.has(subscription)) listeners.add(subscription); // <<< syncSo listeners are actually a collection of refs which point to a callback (forceUpdate). We subscribe in the render phase, however, that doesn't ensure that the callback will be available (render was interrupted / paused / or just on render phase). This handles the case: listeners.forEach(listener => {
if (!listener.current) {
console.warn("Render phase! Interrupted render! ... or unmounting.");
return;
}
listener.current(value);
});Since refs are not set on the render phase (first render phase) the ref will be FYI, I tested the code in codesandbox on will-this-react-global-state-work-in-concurrent-mode and it still passes: 😅 |
|
@eddyw I haven't thought about |
|
The point of Since we're using React.useImperativeHandle(ref, () => ({ setState }))which is perfectly legal. |
Hmm, as I understand the warning will be shown when we invoke listeners in provider, not when we subscribe listeners in children. I'm just guessing though. |
|
https://github.com/facebook/react/pull/17099/files#diff-63c455475b8566c10993b2daa2c3211bR2666
Ok, so maybe we understand the problem in a different way 😛 |
what I understand is it warns the invocation of dispatch (of useReducer). 🤔 |
|
Yes the above should still throw, the way I found that it doesn't is if you call a function that has been bound in render like: https://github.com/JoviDeCroock/hooked-form/blob/master/src/useContextEmitter.ts#L8 |
|
Yeah, useEffect solves my-understanding-of-the warning. My concern is "check4" which I would call parent/child tearing. Probably it would not happen in the hooked-form use case. (I am still interested in how hooked-form is implemented. I could learn real use cases. It would be nice to continue the discussion in #8.) |
Turns out it doesn't solve the issue with this hack. 😕 Now, what's left:
|
|
The third option will be something like a completely different library. The second one may sound good, but it's potentially doing something different. I'd go with the first option with opt-in (the current) update-in-render mode for production. |
|
Just in case, let me ping @acdlite and see if he can give us an advice. use-context-selector/src/index.js Line 58 in a439633 and invoke it from a different component. Is it not recommended in CM? I assume facebook/react#17099 warns this usage. (Well, I actually tried your branch.) |
|
Hm, ok. So yeah, I understood that the warning may be happening if child is in render phase while the action is dispatched 🤦♂
I had an idea, I did: const [prevValue, setPrevValue] = React.useState(value);
React.useEffect(() => {
listeners.forEach((listener) => {
listener(value);
});
setPrevValue(value);
}, [value]);
return React.createElement(OrigProvider, { value: prevValue }, children);Since export const useContextSelector = (context, selector, id) => {
const listeners = context[CONTEXT_LISTENERS];
if (!listeners) {
if (process.env.NODE_ENV !== 'production') {
throw new Error('useContextSelector requires special context');
} else {
throw new Error();
}
}
const initialValue = React.useContext(context);
const [selectedValue, setSelectedValue] = React.useState(() => selector(initialValue));
const selectorRef = React.useRef(null);
React.useImperativeHandle(selectorRef, () => selector);
React.useEffect(() => {
const callback = (nextValue) => {
console.log('[%s] Calling setSelectedValue', id, nextValue);
setSelectedValue((prevSelectedValue) => {
try {
const nextSelectedValue = selectorRef.current(nextValue);
if (nextSelectedValue === prevSelectedValue) return prevSelectedValue;
console.log('[%s] Setting this value:', id, nextSelectedValue, prevSelectedValue);
return nextSelectedValue;
} catch (e) {
console.warn(e);
}
return prevSelectedValue;
});
};
listeners.add(callback);
return () => {
listeners.delete(callback);
};
}, [listeners]);
return selectedValue;
};So |
|
Ok, this is what I did that worked: React.useEffect(() => {
listeners.forEach((listener) => {
listener(value);
});
}, [value]);And: const [startTransition] = React.useTransition({ timeoutMs: 1 });
...
startTransition(() => forceUpdate());However, test3 will fail: In async mode, it seems like there is a kind of race condition if you call |
|
I think it’s the behavior that I’m checking with check4. I haven’t tried if useTransition solves it. |
|
I tried useTransition, but it seems to me that it doesn't improve anything. |
|
I mean, keeping as is, just use |
|
Also, I thought that maybe this will make more sense: React.useEffect(() => {
startTransition(() => {
listeners.forEach((listener) => {
listener(value);
});
});
}, [value]);According to docs, I understand that everything within the transition will make all the updates work like if they were a separe branch. So after the timeout, all components ""should"" technically display the same value. However, I'm away from my laptop to try this right now. I'll see how this works later. |
|
Meanwhile, let me explain why I don't want to use useState + useEffect. const Ctx = createContext(null);
const Children = ({ count }) => {
const value = useContext(Ctx);
if (count !== value.count) {
console.log('tear!!!');
}
return <div>{count}, {value.count}</div>;
};
const Parent = () => {
const [count, setCount] = useState(1);
return (
<Ctx.Provider value={{ count }}>
<button onClick={() => setCount(c => c + 1)}>Click me</button>
<Children count={count} />
</Ctx.Provider>
);
};I didn't run this, but I assume something like this leads to tear with useEffect. |
|
It won't cause tearing 😛
You mention:
I think 2. is rather easy to solve. The main issue is that I believe that listeners.forEach((listener) => {
listener(value);
}); If you make it so |
We can't assume this. Children can re-render at the same time Provider re-renders. const Children = () => {
const [count, setCount] = useContext(Ctx);
const [localCount, setLocalCount] = useState(1);
return (
<div>
<div>{count}</div>
<div>{localCount}</div>
<button onClick={() => { setCount(c => c + 1); setLocalCount(c => c + 1); }}>Click me</button>
</div>
);
};
const Parent = () => {
const value = useState(1);
return (
<Ctx.Provider value={value}>
<Children />
</Ctx.Provider>
);
}; |
|
ash, I was just making a point. Maybe I can summarize all without going in circles 🙈
I made this Sandbox: https://codesandbox.io/s/nice-beaver-9uli8 This is because
|
|
@eddyw Thanks for the summary.
As far as I understand, it doesn't block. (Or we might be using "block" in different meanings.) So, the current code works perfectly in CM. I only care about the warning, and my assumption is that this warning is not applicable to our case because we don't use the forceUpdate's counter value. (It would be really nice if someone can endorse that.) |
|
@dai-shi ok, so I'll leave aside the issues I'm trying to solve for now 😛
Actually, this has nothing to do with using the value, the state is updated regardless. You can test it yourself: let setState = null;
class A extends React.Component {
state = { v: 0 };
render() {
setState = this.setState.bind(this);
return null; // <<<<< not using value
}
}
class B extends React.Component {
render() {
setState({ v: Math.random() });
return null;
}
}
...
<A />
<B /> // << warningWhile a component can update itself in the "render phase" (at least on functional components, is this missing test in React PR?), a component can't or shouldn't update other components state while in "render phase". To be more specific, this is in "initial render phase" before life-cycle methods are attached. If you put a It "may" work as it is right now without displaying warning:
This may help: |
|
@eddyw Welcome back. Now we are discussing the same issue. Uh, probably. Sorry that my wording is a bit unclear.
This is my question. Does this apply to the (If there is an issue even with the
You might miss the point (at least the one I see). The warning is not shown for children subscribing parent, but parent notifying children in our case. |
|
🤦♂ I mentioned:
And point 1 and 2 explain why. However, I decided to do one test more: And it'd actually break, you can think of |
|
Again, this has nothing to do with "using the value". Regardless if you do or not use the value of |
Sorry, I will read it again. Meanwhile,
This is I'm aware of. My question is whether the warning is valid or not in our use case. |
OK, I misread this one. You said "as it is right now".
Yes, I have already confirmed that with one of the examples. Do you think this warning is valid for the use case in this project? |
|
I believe the warning will in fact happen. At least from testing a pseudo implementation using Class components. From what I see in that PR, there is one missing test. I think this relates to a component updating itself in render phase. |
|
I already confirmed this warning in fact happens with the branch in that PR. I'm wondering if I can ignore the warning in our case, or if there's a fundamental issue even with our use case. Naively, it seems OK, but in CM, maybe not? |
This is the final resort, but according to facebook/react#17334 (comment)
I didn't expect the de-opt case flushes passive effects. It's a huge downside. Really want an escape hatch for facebook/react#17099, if we can ignore the warning with our use case. |
|
facebook/react#17099 is merged now. Hopefully, with |
|
Found a solution: useLayoutEffect(() => {
runWithPriority(NormalPriority, () => {
// ...
});
}); |
|
Hmmmm, this solution still leads tearing in useTransition. |
|
I made a proper fix in react-tracked: dai-shi/react-tracked#42 This is not possible with useContextSelector in userland, because update only happens in render. #13 is still a dirty workaround for now, until v2 lands (#12). |
Currently, our Provider invokes subscribed components listeners so that they can force update.
The reason why we need it is that we use the undocumented
changedBits = 0in Context API.use-context-selector/src/index.js
Lines 9 to 15 in a439633
facebook/react#17099 will warn this usage as it does not know about force update.
That's fair. But, unless there's another recommended way, we would need to keep our current approach.
Nobody will use this library if React shows the warning all the time. What should we do?
(Not only this library but also react-tracked and reactive-react-redux.)
The text was updated successfully, but these errors were encountered: