meteor / react-packages

Meteor packages for a great React developer experience
http://guide.meteor.com/react.html
Other
571 stars 157 forks source link

react-meteor-data v2.2.1 - unnecessary renders #313

Closed robwheatley closed 3 years ago

robwheatley commented 3 years ago

Perhaps I'm missing something here, but I'm having a spot of bother with v2.2.1. I'm new to react, Javascript and Meteor, so please don't bite!

I recently updated my project to Meteor 2.0. After seeing all the warnings in the logs from withTracker RE undefined deps array, I separately updated react-meteor-data to 2.2.1 as the warnings were due to a bug that was fixed in 2.2.1

I'm using useTracker to tell me when a Meteor subscription is ready, like this:

  const projectsReady = useTracker(() => {
    const subscription = Meteor.subscribe('projects')
    return subscription.ready()
  }, [])

But when using Meteor Dev tools to watch DDP messages, I see that there are 2 'Subscribing to projects' going off to the server. I don't really see why as I have an empty dependancy array rather than no dependancies. Sure enough, logging on the server shows me that 2 subscription requests arrived there. As a note, the subscription handles I get back had different IDs, but I thought that Tracker.autorun took care of that behind the scenes (looking at Meteor Docs)?

Just to be silly, I did this to see what would happen:

  useTracker(() => {
    console.log('Hello world');
  },[])

And, I get 'Hello world' in my log twice (the function is not rendering due to a props change, BTW).

Final note: this didn't used to happen in version 2.1.1 (98% sure of that).

My concern is is two fold - First the extra Subscribes (and corresponding unsubscribes that happen right after) and also the unnecessary renders this causes.

Am I missing something? Is it a bug? Something else?

Looking forward to an expert opinion.

CaptainN commented 3 years ago

Subscriptions are a little bit of black magic in Meteor. One of the things that changed in 2.2.1 is that I removed a lot of code which was designed to keep the initial computation (a side effect), created during render alive after the component mounts. This was causing some rare edge case failures with Subscriptions, but it was also making the code base super complex, and difficult to maintain.

All that was replaced with a system which runs and disposes of the computation on render (to avoid creating lingering side effects) and then reruns the computation on mount (in useEffect).

That said, Meteor's subscription black magic should be retaining the data loaded in the initial subscribe call even though it subscribes again, without causing all that data to load a second time. It should be recycling the subscription, or at least the data in the subscription. If that's not happening, we may need to take another look at this.

This does mean that it's rendering a second time, on every render. The React team has been stressing that extra renders aren't really that big of a deal. They should be quick and disposable, they say. The new hook is meant to honor this philosophy. It is annoying that it has to render a second time, but it's necessary to support things like concurrent mode properly.

If you want something super extra optimized, a few of us have been working a pair of alternative hooks, which will provide a semi-immutable data source for for find queries, and a possibly more optimized subscription hook (though I'm having trouble getting the tests to work). It might be worth checking those out when we enter alpha/beta on those. You can checkout progress on that in PR #298

robwheatley commented 3 years ago

Thanks for the reply. I'll check out that PR. In the mean time, I have a little more info to add that may influence things a little..

Oh, and one thing I forgot to add, I'm also using React-Router (I seem to remember that gave you a little grief).

In the code above, you can see that I'm using useTracker to get a value for projectsReady. Whilst that is false, I show a spinner. Once it becomes true, I load my app for real. ie, I don't want to load my app until the subscription is ready (actually a few subscriptions, but that doesn't matter for this example).

A component lower down in the app is tracking the number of projects, like this:

  const projectCount = useTracker(() => {
    return Projects.find({}).count()
  }, [])

In reality, the tracker is waaaay more complicated than this, but I tested this simple tracker and I got the same problem.

I want this component to re-render if the count in Projects changes. Simple enough.

However (and this was driving me insane) no matter how the projects collection changes, the count tracker wasn't triggering.

I had a thought that this maybe due to the 'cancelled subscriptions' from the subscription useTracker (in my first post) firing too many times right at the very beginning.

I got my first clue by removing the dependancies array in the projectCount tracker. I got my second clue by adding a dependancy that I changed elsewhere (which forced a recalculation of the tracker after the initial render). Both of these things worked (ie, I would be told of a count change). However, in my real app, I have a list of dependancies that don't change on initial page load, but are needed, so I couldn't use either of these 2 solutions.

So, I did this: When the 1st tracker tells me that the subscription is ready, I don't immediately render the app. Instead I wait 200ms, then render. This solves the problem. The count tracker now works. I think because it is tracking the 'proper' subscription, not the unsubscribed one. Although, like you say, this should not be a problem given all the Meteor black magic that goes on.

Specifically, I do this:

Given the state change and re-render, I could probably get away without a timer. I know that it still works at 20ms for instance. Tomorrow I might try removing the timer and see if the extra state change alone is enough.

In my app, this 200ms delay is not a big deal. It will only happen once either on login or on a forced page re-load.

Anyway, I thought I would mention it, as I'm likely not going to be alone with this problem. People may be scratching their heads wondering why their trackers are no longer tracking in certain circumstances...

CaptainN commented 3 years ago

Thanks for the detailed response. I'm actually curious if any smaller value would work. 0ms, 1ms, or 2ms.

This should create a reactive computation, regardless of what the subscription is doing, or whether it was mounted before or after the subscription:

const projectCount = useTracker(() => {
    return Projects.find({}).count()
  }, [])

That it's not, is concerning.

CaptainN commented 3 years ago

If you could, please try this version of useTracker (put it in a local js file somewhere, and import from there instead of meteor/react-meteor-data):

export const useTracker = (reactiveFn, deps) => {
  let [data, setData] = useState();

  useMemo(() => {
    // To jive with the lifecycle interplay between Tracker/Subscribe, run the
    // reactive function in a computation, then stop it, to force flush cycle.
    const comp = Tracker.nonreactive(
      () => Tracker.autorun((c) => {
        if (c.firstRun) data = reactiveFn();
      })
    );
    // To avoid creating side effects in render, stop the computation immediately
    Meteor.defer(() => { comp.stop() });
    if (Meteor.isDevelopment) {
      checkCursor(data);
    }
  }, deps);

  useEffect(() => {
    // NOTE: Wrapping this in nonreactive to test an idea
    const computation =  Tracker.nonreactive(
      Tracker.autorun((c) => {
        setData(reactiveFn(c));
      })
    );
    return () => {
      computation.stop();
    };
  }, deps);

  return data;
}

I have one other idea if this doesn't solve it.

robwheatley commented 3 years ago

Quick note, I deleted a comment I added earlier about things now working even without my hack. It's not as simple as that, so please ignore the mail you would have received.

Here is the latest. I have tested different timers on my little 'delay' function.

I can get the timer all the way down to 0ms, and things work. Meteor.setTimeout(()=>setOkToGo(true), 0)

But if I remove the timer altogether (just set the state) then they don't work - my trackers tracking collections are not reactive. So, just the extra delay setting a 0ms timer is enough to 'solve' the problem, but not a state change on its own.

As a note, this app is running on localhost.

On the local useTracker code you provided. I gave it a go, but it complained about checkCursor not being defined - not knowing what I was doing, I copied that function from the original react-meteor-data file and put it in the local useTracker version, but now it's complaining about a type error (f is not a function, tracker line 603 being called from the useEffect in the code you provided). Clearly I need to do something different.....

CaptainN commented 3 years ago

If you would, remove that check, and give it a try - it's only useful to provide development experience, but we don't need it for this test.

export const useTracker = (reactiveFn, deps) => {
  let [data, setData] = useState();

  useMemo(() => {
    // To jive with the lifecycle interplay between Tracker/Subscribe, run the
    // reactive function in a computation, then stop it, to force flush cycle.
    const comp = Tracker.nonreactive(
      () => Tracker.autorun((c) => {
        if (c.firstRun) data = reactiveFn();
      })
    );
    // To avoid creating side effects in render, stop the computation immediately
    Meteor.defer(() => { comp.stop() });
    // if (Meteor.isDevelopment) {
    //   checkCursor(data);
    // }
  }, deps);

  useEffect(() => {
    // NOTE: Wrapping this in nonreactive to test an idea
    const computation =  Tracker.nonreactive(
      Tracker.autorun((c) => {
        setData(reactiveFn(c));
      })
    );
    return () => {
      computation.stop();
    };
  }, deps);

  return data;
}
robwheatley commented 3 years ago

With that check removed, I still get the same problem of:

Uncaught TypeError: f is not a function
    at Object.Tracker.nonreactive (tracker.js:603)
    at local-useTracker.js:24

local-useTracker is my file name for the file you provided, and my line 24 is this line from your code:

 const computation =  Tracker.nonreactive(
      Tracker.autorun((c) => {
        setData(reactiveFn(c));
      })
    );

Whether this was the right thing to do or not, I changed your code to this:

    const computation =  Tracker.nonreactive(()=>{
      Tracker.autorun((c) => {
        setData(reactiveFn(c));
      })
    });

And now things are working as expected, ie, I have reactivity back. No idea of the consequences of what I just did, and I've not got the time right now to look into it. I'll have more time tomorrow, but I guess that it's the same as running the original code with no dependancy array..?

CaptainN commented 3 years ago

The consequences of that change would be that it'll never stop your computation on unmount (which is probably why the reactivity returned). I'm tracking a separate set of issues with the deps array not being applied correctly. I wonder if you are having a similar problem to that. Needless to say, I'm working on it.

robwheatley commented 3 years ago

OK, thanks for the update - my day job has taken over at the moment, so I'm likely not going to get back to my app until the weekend. Perhaps I can add more then.

CaptainN commented 3 years ago

Try this one when you get a chance

const useTrackerWithDeps = (reactiveFn, deps) => {
  let [data, setData] = useState();

  const { current: refs } = useRef({ reactiveFn });
  refs.reactiveFn = reactiveFn;

  useMemo(() => {
    // To jive with the lifecycle interplay between Tracker/Subscribe, run the
    // reactive function in a computation, then stop it, to force flush cycle.
    const comp = Tracker.nonreactive(
      () => Tracker.autorun((c) => {
        if (c.firstRun) data = refs.reactiveFn();
      })
    );
    // To avoid creating side effects in render, stop the computation immediately
    Meteor.defer(() => { comp.stop() });
  }, deps);

  useEffect(() => {
    const computation =  Tracker.nonreactive(
      () => Tracker.autorun((c) => {
        setData(refs.reactiveFn(c));
      })
    );
    return () => {
      computation.stop();
    }
  }, deps);

  return data;
}
robwheatley commented 3 years ago

@CaptainN Just did a quick test of the latest code you provided. That works. Reactivity restored.

robwheatley commented 3 years ago

I've also discovered why the tracker was subscribing to the collections twice. It's nothing to do with useTracker, it's was me being an idiot! (I wrote a little function on the Route to check that the user was authenticated and that was forcing the creation of a new react element, hence 2 calls).

So, the first part of my first post is no longer valid. Still hoping that some good will come of this thread though (in terms of helping get to the bottom of last reactivity) ;-)

filipenevola commented 3 years ago

This issue should be fixed on 2.2.2 released just now.

Thank you @CaptainN

minhna commented 3 years ago

@CaptainN I'm running Meteor 2.2 with react-meteor-data@2.2.2 and "react": "^16.13.1", The "double renders" issue is still there.

import React, { useEffect, useState } from 'react'
import styled from 'styled-components'
import { useTracker } from 'meteor/react-meteor-data'

import { Typography, Button } from '@material-ui/core'

const StyledTestUseTracker = styled.div``

function TestUseTracker() {
  const [n, setN] = useState(0)

  useTracker(() => {
    console.log('in tracker', { n })
  }, [n])

  useEffect(() => {
    console.log('in effect', { n })
  }, [n])

  return (
    <StyledTestUseTracker>
      <Typography variant="h2">TestUseTracker</Typography>
      <div>current number: {n}</div>
      <div>
        <Button onClick={() => setN(n + 1)} variant="contained">
          (+) Increase
        </Button>
      </div>
    </StyledTestUseTracker>
  )
}

export default TestUseTracker

With that component, when I click the button to increase the number, I can see the console output:

in tracker {n: 3}
in tracker {n: 3}
in effect {n: 3}