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

[s7dhansh]

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: https://github.com/facebook/react/issues/18178

The warning occurs because along with running the function in useEffect, useTracker also runs it inside useMemo.

What shall I do? Is there a different design pattern that I should follow, or is there a way to fix it in useTracker?

[s7dhansh]

@filipenevola @benjamn any help?

[CaptainN]

I think the react team doesn't want you to do that. Why are you doing it this way?

[s7dhansh]

It is a common pattern to update a parent component's state from a child component. React team does not restrict us, only says that we have to do it inside useEffect, for improved clarity and easy debugging.

The problem here is when we do such a state change inside useTracker, we get a warning because the argument function is run inside useMemo as well, along with useEffect.

I was wondering if there is a way to fix this in useTracker. If not, then the only solution I can think of is having an extra state in the child component, setting it inside useTracker and then using an additional useEffect that depend on this extra state, and change the parent's state inside this.

[CaptainN]

I still question why you would update this way. I'll look though. It might be possible to avoid setting state in useMemo.

[s7dhansh]

Example: I create a top level store Context. I set some data in the store that can be used by other components. For instance, Meteor.user().

While it can be argued that I can always directly use useTracker in the sibling components as well, but it becomes tedious and expensive if that data is transformed heavily. It can also be argued that I create a wrapper component over all such sibling components and then pass the data as props. But I have a few components that can be re-used at various nested levels and it makes much more sense to get the data from store rather than from prop inheritence.

[dburles]

I’d just make a useUser hook instead of using context.

[CaptainN]

@s7dhansh I'm just not sure how what you've described turns in to the problem the react note describes. I've used useTracker from inside a context provider, paired with a context consumer many times, without any trouble. Are you doing something like setting state from within the useTracker computation?

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.)

[CaptainN]

So, we are not actually setting any state inside useMemo or in the hook body before the component is mounted, though we do call the user's computation function inline. I'm guessing you are calling setState yourself? Ultimately, it's on you to avoid side effects in this case.

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 useEffect (this is your best bet) or if you are careful enough about the lifecyle (you use deps precisely right - or empty deps if you can) you can avoid setting state on the computaton's firstRun by checking for that on the computation in your computation handler:

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.

[s7dhansh]

Thank you for taking your time to look at this. c.firstRun will not solve this issue as useMemo is not limited to the firstRun. For now, I am storing it separately and modifying the state in a useEffect as I mentioned above.

const obj = useTracker(() => {return .....}, []);
useEffect(() => {setState(obj);}, [obj]);

I completely agree that hooks are a better way for simple things like Meteor.user(). My use case is bit different though. Will try to prepare a relevant reprod and share. But for now, I will close this as there is a decent workaround.

[CaptainN]

@dburles @menelike @yched @filipenevola Do you think the above is worth documenting more carefully? It's not only that, but also that users of useTracker should not create side-effects on first render (before the render is "committed"). This is actually a tricky topic, and we cannot create a technical "fix" for this, without creating/offering an alternative "pure" hook implementation (a null result until useEffect is run after the render is committed) - and I increasingly think we should.

[menelike]

@CaptainN

I am not really sure what the problematic scenario is. As @s7dhansh has suggested I'd like to see a reproduction before assuming anything.

This is actually a tricky topic, and we cannot create a technical "fix" for this, without creating/offering an alternative "pure" hook implementation (a null result until useEffect is run after the render is committed) - and I increasingly think we should.

I agree. The way useTracker has been implemented is not the way how react works nowadays, it's just a question of time until it fails unfixable. It makes sense to add an additional hook as you suggested - but most importantly keep the current behavior in useTracker always backward compatible. Users should be encouraged to use the new hook instead and stop adding technical debt to their code.

[s7dhansh]

@menelike the problematic scenario is

const obj = useTracker(() => {
 setState(xyz);
 return xyz;
}, deps);

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.

[dburles]

@dburles @menelike @yched @filipenevola Do you think the above is worth documenting more carefully? It's not only that, but also that users of useTracker should not create side-effects on first render (before the render is "committed"). This is actually a tricky topic, and we cannot create a technical "fix" for this, without creating/offering an alternative "pure" hook implementation (a null result until useEffect is run after the render is committed) - and I increasingly think we should.

I think this new hook might be of use for a future version https://github.com/facebook/react/pull/18000

[dburles]

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!

[menelike]

I am having a hard time understanding why it is necessary to use setState inside the reactive function. I agree with @CaptainN that the pattern itself might be the problem.

const obj = useTracker(() => {return .....}, []);
useEffect(() => {setState(obj);}, [obj]);

looks like the valid solution.

Additionally, this reminds me of onSubscriptionDatafrom https://www.apollographql.com/docs/react/data/subscriptions/#usesubscription-api-overview.

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 useEffect(() => {onSubscriptionData(refs.trackerData);}, refs.trackerData);, so nothing really won.

[dburles]

@dburles @menelike @yched @filipenevola Do you think the above is worth documenting more carefully? It's not only that, but also that users of useTracker should not create side-effects on first render (before the render is "committed"). This is actually a tricky topic, and we cannot create a technical "fix" for this, without creating/offering an alternative "pure" hook implementation (a null result until useEffect is run after the render is committed) - and I increasingly think we should.

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 ...
};

[menelike]

@dburles

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 withTracker. Or in other words, your code must suddenly deal with cases where even if the data is available in MiniMongo, useTracker returns undefined.

[CaptainN]

@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 useMutableSource hooks is the way forward (and I'm glad the React team accepted the need for this - it's not just Meteor, but MobX and others that need it - even Apollo probably needs it - their hook will leak memory in some scenarios). My guess is, without diving in deeply to that thread, that it will deop a component which uses that hook (makes the component run like it's not in concurrent mode). This is the right solution, and whenever that ships, I'll update the hook.

@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 useMutableSource ships anyway), are challenging. It's ALWAYS okay to create side effects when c.firstRun is false, but it's only okay to create side-effects when c.firstRun is true after the very first c.firstRun - and that's difficult to detect (and reason about), because in concurrent mode, there are multiple first c.firstRuns!

[CaptainN]

@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 observeChanges instead of using a computation.

[menelike]

When I grow up I want to explain subjects as profound and proficient as @CaptainN 💯

[dburles]

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 useTracker will initially be undefined on the client:

  const { loading = true, books = [] } =
    useTracker(
      () => ({
        loading: !Meteor.subscribe('books', limit).ready(),
        books: Books.find({}, { limit }).fetch(),
      }),
      [limit],
    ) || {};

[CaptainN]

@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.

1. There is currently no real problem with actually creating side effects in render, as renders are all run synchronously (with some caveats for ErrorBounds and Suspense). This is a little bit of a dirty secret. The "rules of hooks" are guidelines that are meant to protect you from future changes. That fact, mixed with the fact that Meteor's reactive sources are often available immediately, means that conceptually, we should have access to these APIs immediately, darn it! That is, why bow to React team preferences, when our APIs are perfectly capable of supplying data immediately? That may sound petty, but more in a minute (when discussing item 3 below).

2. Backwards compatibility. The new hook is used to implement an updated withTracker, and that component expects data immediately. This behavior is required for back compat.

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.

1. The current implementation. Use timeouts to manage destroying uncommitted computations in renders that are tossed when using Concurrent Mode. This is super hard to choreograph, and even harder to read the result in code. But it does work, and is completely compatible with the fast paths in React's Concurrent Mode, and avoids rebuilding computations after mount - in most cases. It's worth noting that we aren't the only ones doing this crazy stuff with timeouts - even useSubscription eventually boils down to timeouts (at least the last time I looked at it it did).

2. Switch to using useMutableSource. This is the cleanest way to implement exactly what we want. But it also deopts the React pipeline - basically, if mutable data is detected, it switches out of concurrent mode, and back to synchronous mode rendering (by throwing to enqueue a new render), for components that use the hook.

3. A new implementation that just doesn't run the user's function in render. This is also clean, however, it has some drawbacks. It sort of subverts the entire intent behind concurrent mode. If your component just renders "null" or a loading component for every first render pass - what benefit did you really gain from concurrent mode? That first pass will not give the render engine a realistic representation of performance, since you'll get an entirely different tree after the render is finally committed. I bet this is actually slow than 2 in practice (because it would introduce the delayed rendering from concurrent mode, without actually reducing jank).

[dburles]

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.