-
Notifications
You must be signed in to change notification settings - Fork 157
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
React 16.13.0 Warning: Cannot update a component from inside the function body of a different component #286
Comments
@filipenevola @benjamn any help? |
I think the react team doesn't want you to do that. Why are you doing it this way? |
It is a common pattern to update a parent component's state from a child component. The problem here is when we do such a state change inside I was wondering if there is a way to fix this in |
I still question why you would update this way. I'll look though. It might be possible to avoid setting state in useMemo. |
Example: While it can be argued that I can always directly use |
I’d just make a |
@s7dhansh I'm just not sure how what you've described turns in to the problem the react note describes. I've used Maybe a code snippet or two would help illustrate the issue. (I'm going to look at whether it's feasible to not set state in useMemo, but I'd still like to understand the issue.) |
So, we are not actually setting any state inside If you want to avoid setting state on the first render, you can do as you suggested and keep track of whether the component has committed yourself with useTracker((c) => {
if (!c.firstRun) setState('blah')
}, []) But I still wonder about the approach you are using. Composing hooks seems like a better way to go, rather than updating parent state from child components. |
Thank you for taking your time to look at this.
I completely agree that hooks are a better way for simple things like |
@dburles @menelike @yched @filipenevola Do you think the above is worth documenting more carefully? It's not only that, but also that users of |
I am not really sure what the problematic scenario is. As @s7dhansh has suggested I'd like to see a reproduction before assuming anything.
I agree. The way |
@menelike the problematic scenario is
The reprod I am planning to prepare is just for inputs on my specific use case and if there is a better way to do it. |
I think this new hook might be of use for a future version facebook/react#18000 |
As a side note, a long time ago I remember being frustrated at the Tracker API being at odds with React and experimented with this package, which allowed you to use Tracker in a functional way https://github.com/dburles/meteor-conduit Here's an example using it with React and Redux: https://github.com/dburles/conduit-example/blob/master/client/imports/containers/App.jsx FWIW my point is that I wouldn't hesitate altering the API greatly to better accomodate Tracker in context of React and hooks, considering the API hasn't changed from the HoC! |
I am having a hard time understanding why it is necessary to use
looks like the valid solution. Additionally, this reminds me of But I don't think adding an event-based callback is easy to implement in this scenario, at least if it needs to be called synchronously on the result of the reactive function. The implementation might look like |
Sorry to hijack the issue, @CaptainN @menelike I'm interested in where this naive implementation breaks down as I don't quite understand why it's necessary to run before render. const useTracker = (fn, deps = []) => {
const [state, setState] = useState();
const memoizedFn = useCallback(fn, deps);
useEffect(() => {
const c = Tracker.autorun(() => setState(memoizedFn()));
return () => c.stop();
}, [memoizedFn]);
return state;
};
const Page = () => {
const [limit, setLimit] = useState(PAGE_BY);
const { loading = true, books = [] } =
useTracker(
() => ({
loading: !Meteor.subscribe('books', limit).ready(),
books: Books.find({}, { limit }).fetch(),
}),
[limit],
) || {};
return ...
}; |
Every time I stop working on the current implementation for a couple of days I totally forget all the reasonings behind the code. It was a mind-twisting experience. Regarding your example (and @CaptainN please dis/approve), the returned state might be undefined on the initial render which would have introduced a new behavior in contrast to |
@dburles @menelike covered it well, but on top of getting that initial undefined value and the problems @menelike described with backward compatibility, in order to make this work in SSR, we'd have to have divergent behaviour. In SSR, it would still have to immediately return data. BTW, I think the typescript implementation of the hook (in PR #283) is easier to read and understand. It's basically done - I just have to finish fixing the unit tests. That @dburles Not hijacking - I'm glad to have the eyes on this. Regarding the issue at hand - @menelike covered it - we should make a note about side effects inside the tracker, and discourage that altogether. It's always okay to set state on the current component, and propagate that down, but really, no one should be propagating state up - just compose the hook instead (and if it's a performance concern, use the hook in a context provider, and create a context consumer hook - this pattern is described in the blog article announcing the hook). Setting state on the parent component is a lot like creating side effects. There are some advanced side effects scenarios that might be possible. We actually have a computation lifecycle handler - but it's currently undocumented (used for some internals validation in unit tests), but it could conceivably be used for creating side-effects and handling cleanups. The problem is that the rules about when to create side effects (until |
@dburles I've also been thinking of much more integrated ways to write a set of hooks for Meteor with React. @menelike have actually gone back and forth on that idea - I'll checkout out conduit, it's looks neat. It seems we should be able to iterate on the Cursor more tightly integrated in React (kind of like Blaze does) instead of using a computation through Tracker, so that if a single document changes in a data set (or maybe even a single property?) we only have to invalidate the one component instance, and avoid rendering the entire array for every change. I also wonder whether we might be able to get around some of these "mutable sources" problems if we use |
When I grow up I want to explain subjects as profound and proficient as @CaptainN 💯 |
Thanks @CaptainN, though I still don't quite understand where my implementation would fall apart. I don't see the missing values as too much of a problem and if it makes things simpler, release as a breaking change in version 3. Also, SSR returning data immediately should also not be a problem. You would just have to write around the fact that const { loading = true, books = [] } =
useTracker(
() => ({
loading: !Meteor.subscribe('books', limit).ready(),
books: Books.find({}, { limit }).fetch(),
}),
[limit],
) || {}; |
@dburles I like that clean implementation, and actually, I think we should ship that as an option for those who want it (item 3 below). But there are two problems, and they are both debatable.
Now, there are pure ways to implement the hook, while also staying in-line with the rules of hooks. In fact, the no-deps implementation of the hook is currently "pure" in that it follows the rules of hooks precisely. What it does is, runs the user reactive func inline in a computation, then destroys the computation right away (to stop any subscriptions, etc.). Then in useEffect, it starts it all up again. This is 100% in line with the rules of hooks. The deps version is trickier, and is where we wanted to make improvements, and recycle the computations, etc. I noticed a significant drop in performance from double querying my databases without deps early on (with @yched's early implementation, which now almost exactly matches the current no-deps implementation). That's where all this effort came from. That leaves us with 3 options, all with pros/cons.
|
Thanks @CaptainN that overview makes it super clear. So it seems like a lot of the complexity is primarily around supporting both sync and concurrent mode. Perhaps the root of the problem is that the current API is at odds with how data loading is ideally implemented in concurrent mode? it's got me thinking about what that API could look like. I'd like to chat about it some more if you have time, I've just sent you a message on Meteor Community on Slack. |
I use a child component to change the state of its parent using
useTracker
.With the new React version, it causes this warning https://reactjs.org/blog/2020/02/26/react-v16.13.0.html
Relevant issue: facebook/react#18178
The warning occurs because along with running the function in
useEffect
,useTracker
also runs it insideuseMemo
.What shall I do? Is there a different design pattern that I should follow, or is there a way to fix it in
useTracker
?The text was updated successfully, but these errors were encountered: