meteor / react-packages

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

Setting state within useTracker causes effects to run reactively #317

Closed edemaine closed 3 years ago

edemaine commented 3 years ago

Consider the following code:

defaultPage = new ReactiveVar();
function App() {
  [page, setPage] = useState();
  useEffect(() => {
    console.log(Tracker.currentComputation);
    const observer = Objects.find({page: page}).observe({added: ...});
    return () => observer.stop();
  }, [page]);
  useTracker(() => {
    if (!page) {
      setPage(defaultPage.get());
    }
  }, [!page]);
  ...
}

I find that the useEffect callback is run within a Tracker (the console.log outputs an actual computation), presumably triggered by setPage called within useTracker. I tried wrapping setPage within Tracker.nonreactive, but the effect callback is still called reactively. This seems wrong...

The result of this is pretty annoying. The tracker actually gets called twice, and because of the if, only the first one triggers the observer to be created. Because the observer was created within a tracker, the second call of the tracker invalidates the first call so the observer gets destroyed and not recreated. 🙁

I tested in react-meteor-data 2.1.4, 2.2.1, and 2.2.2 (wondering whether the recent rewrite had an effect) and both have the same issue. I'm using React 17.0.1

CaptainN commented 3 years ago

I don't think you should be mixing APIs in this way. Maybe we should make that clearer in the docs.

Is there a reason you can't just return the data you want from useTracker? Why are you calling setPage from in there?

Also, your useEffect example has no deps set, which means it'll run after ever render. You probably didn't intend that.

edemaine commented 3 years ago

The example is a simplified illustration of a much more complex app... The motivation is that useEffect is useful for waiting for the DOM to render (whereas useTracker's first run is before). In my actual application, I want to instantiate a complex class that needs both a pageId and DOM refs, so this seemed like a natural solution. That class happens to build an observer.

In the spirit of not mixing interfaces, I guess I could replace the useState with a ReactiveVar and a useTracker to read it, and pass that into the effect. That would be a nicer workaround than my current workaround (wrapping the useEffect callback in Tracker.nonreactive, which is pretty confusing because it's not naturally reactive).

Still, if it's technically possible, it would be nice if React state could be modified in a useTracker; it's certainly not intuitive that it can't. I'm particularly confused why Tracker.nonreactive(=> setPage(...)) doesn't work. Presumably setPage queues a React update to happen later, but I don't understand why it's run reactively within a tracker. If it's not technically possible, mentioning this in the docs would be nice.

(You're right about the missing deps; fixed, sorry. And thanks for the quick response!)

CaptainN commented 3 years ago

Tracker.nonReactive is meant to wrap Tracker.autorun, not specific functions within a computation's reactive function body.

You really shouldn't need to call a state setter from within your tracker's reactive function. Just call it after the function returns if you really need to. But you generally shouldn't need to - useTracker will already trigger rerender when you need it to.

defaultPage = new ReactiveVar();
function App() {
  const {object, page} = useTracker(() => {
    const page = defaultPage.get();
    return {
      page: page,
      object: Objects.find({page: page}).findOne();
  });
  ...
}
edemaine commented 3 years ago

Agreed, it's possible to rewrite code to avoid this. It just feels like an artificial limitation... If you look at my examples above, setState is called within an if condition, and it's annoying to factor that out into a return value that gets passed into a useEffect. Your example always sets page to defaultPage.get(), but my original strawman only did that when page was false, and it's crucial to allow the page to get set to things other than defaultPage.get() -- that's just an initializer.

As I mentioned, I need a useEffect (not useTracker alone) so that the DOM is rendered and refs are set. If there were a useTrackerEffect or similar that gets called after the render instead of before, I could avoid a lot of this complexity...

CaptainN commented 3 years ago

It's not an artificial limitation - these are two different state tools. useTracker does everything you need with regard to Meteor's reactive data sources. You really don't need to, and in 99% of cases, probably shouldn't, mix these state managers.

That said - there isn't anything really stopping you from doing it - in some cases, it can work. But there are other issues with your code snippet - for example, you added a deps array to your useEffect, but in your useTracker deps, you are passing a negated non-boolean value, which is not quite right.

edemaine commented 3 years ago

Fair enough. It still feels dangerous to me that this is possible — it's pretty weird that the useEffect runs within a tracker — but I guess this is just a consequence of two powerful magics (React's job queue and Meteor's reactivity/tracker) being unaware of each other. I tried looking into React's source code to track down exactly how this happens, but it's difficult going...

I took a stab at writing a warning in the documentation (#320).

in your useTracker deps, you are passing a negated non-boolean value, which is not quite right.

This is intentional. I only wanted the useTracker to reset when the truthiness of page changes, in particular when it changes between undefined/null and an actual page. Note that the useTracker code only uses !page, not page directly.

filipenevola commented 3 years ago

I believe the PR updating the doc is enough here, right?

I'm closing it.