Meteor and React V18 incompatibility

[KrisjanisStrods]

I have created the following project, to highlight the issue when trying to update React to V18.
https://github.com/KrisjanisStrods/meteor-and-react-V18-incompatibility

Seems to me, that meteor/react-meteor-data does not support React V18.

[CaptainN]

I'll try to take a look soon.

One note, you really shouldn't do this - it means the find() function on every pass through the function, which can have an effect on the reactivity of that function. In general, you want reactive function to be called every time the computation runs. This is similar to the "rule of hooks" where you should not put hooks in a control statement. If you run that without the loading check, you should just get an empty array - but it won't error.

[CaptainN]

I can validate this is a problem. The tests are failing in a colossal way. 😓

I'll look in to it.

[CaptainN]

So this is only happens when using useTracker without deps. I did manage to get it to work, by essentially using a method for cleanup more similar to the method in useTrackWithDeps. But, there's a reason we didn't use that before - I'm not sure it'll work with older versions of React. IDK, maybe it'll work.

This does cause some other issues, with events firing out of order. I also found an opportunity to do something we haven't done since a very old version, which is to keep the initial computation in some cases. BUT, that throws off a lot of the tests, and I don't know if it'll be reliable. I'm actually pretty it will not be. I'll need to clean out a lot of tests, if we do it this way (that's probably fine, these are a little too rigid, and brittle).

I'll try to get to this this weekend.

[CaptainN]

The main problem I'm having with the tests is that the testing platform was updated, and that update is required for using React 18. That update is not backward compatible, so it's going to be tricky to test both React < 17 and React 18+.

It looks like things are actually working, but I need to spend some time to get the tests working again.

In the mean time, I'll push up a branch over the weekend, so folks can check out the current progress.

[rijk]

@CaptainN wondering if what you're mentioning is related to this:

In the future, we’d like to add a feature that allows React to add and remove sections of the UI while preserving state. For example, when a user tabs away from a screen and back, React should be able to immediately show the previous screen. To do this, React would unmount and remount trees using the same component state as before.

This feature will give React apps better performance out-of-the-box, but requires components to be resilient to effects being mounted and destroyed multiple times. Most effects will work without any changes, but some effects assume they are only mounted or destroyed once.

To help surface these issues, React 18 introduces a new development-only check to Strict Mode. This new check will automatically unmount and remount every component, whenever a component mounts for the first time, restoring the previous state on the second mount.

Would this cause issues for useTracker (with/without dependencies) and the micromanaging of lifecycle we do in those hooks?

[CaptainN]

We have supported that (at least, in earlier iterations) for a while. Supporting that is the reason everything double renders (although, I might make an attempt to reduce that - the behavior in 18 is a little different than the last time I looked at it - it seems more stable, which is guess what you might expect).

Basically, you really don't want to create side-effects in render - or if you do (like we have to), you just need to clean them up effectively when it goes stale (which is what we do).

It makes things tricky because of some magic that Meteor's subscription system does behind the scenes to recycle subscriptions. A lot of this would be simpler if we didn't have to support that. Honestly, this entire pipeline should be reconsidered. We might want to revisit a custom useSubscribe hook, but dig a bit deeper in to the implementation in the DDP side.

[rijk]

A lot of this would be simpler if we didn't have to support that. Honestly, this entire pipeline should be reconsidered. We might want to revisit a custom useSubscribe hook, but dig a bit deeper in to the implementation in the DDP side.

Maybe we can revisit my concept for a useSubscribe that Suspends and create React 18-optimised version of the hooks (that has a peer dependency on ^18)?

From what you're saying, it sounds we'll get double benefits; Suspense support for subscriptions (allowing you to simplify your component code), and a simpler useTracker implementation.

[CaptainN]

It should actually be possible to support Suspense from within useTracker (and error boundaries). For example, if the subscription isn't ready, we should be able to support a throw directly from there, and pass that up to a Suspense handler.

What I would recommend for your custom useSubscription is that you look in to the tests we have for useTracker, and see if those work with your solution. Those are really old tests (they predate my involvement), which can reveal some of those "magic" subscription edge cases that I mentioned. When I restored those in this project a year or so ago, they revealed some real issues with random subscription dropping and other problems like even a rapid sub/unsub loop that would occur sometimes. That's the headache I'm mostly talking about.

The main problem, even with suspense, is that it's the interaction with react's lifecycle, and whatever Meteor's subscribe is doing inside that causes these very hard to test for edge cases. Suspense doesn't really solve that on it's own - I really think it would take a deeper dive in to whatever is going on inside the core subscribe function.

[CaptainN]

BTW, I do like the idea of supporting Suspense for data loading properly, but I've been waiting for React to send a signal that Suspense is ready to be used for that purpose. Previously, React has made it clear that using Suspense for data is not a supported use for it. It also didn't have support in SSR. It was originally meant to be used for loading components with React.lazy, and didn't have any SSR support. I don't know if those requirements have changed with React 18, but I'll find out.

[CaptainN]

I spent a bunch of time over the weekend trying to update the tests to support the change to v18 - it's a lot of work. I'm going to push up the working hook to a PR, and worry about the tests later. It's important to get those working before we release, but the testing platform is proving to be a challenge to get working right.

[j18ter]

I'm going to push up the working hook to a PR, and worry about the tests later.

Which branch has the working hook for React v18?

[CaptainN]

It's up now on react-18 and PR #359

[CaptainN]

This is out on version 2.5.0 - has it fixed the issue?

[StorytellerCZ]

@KrisjanisStrods

I had no problems so far.

[wolasss]

Unfortunately the version 2.5.0 breaks my app:

1. You might have mismatching versions of React and the renderer (such as React DOM)
2. You might be breaking the Rules of Hooks
3. You might have more than one copy of React in the same app
See https://reactjs.org/link/invalid-hook-call for tips about how to debug and fix this problem.
    at Object.throwInvalidHookError (modules.js?hash=d1e0e625f0f3e261a711d5ad37cff7a53f431a80:21902:9)
    at useContext (modules.js?hash=d1e0e625f0f3e261a711d5ad37cff7a53f431a80:4265:21)
    at index.tsx:99:18
    at withTracker.tsx:19:15
    at useTracker.ts:69:18
    at Computation._compute (tracker.js:308:7)
    at Computation.Tracker.Computation._compute (tracker.js:49:24)
    at Computation._recompute (tracker.js:324:16)
    at Object.Tracker._runFlush (tracker.js:495:14)
    at Object.Tracker._runFlush (tracker.js:13:23)```

It basically breaks when using `useContext` in `wtihTracker`. This happens no matter if I use react v17 or v18. I'll try to create a separate repo so that this error can be reproduced. 

[StorytellerCZ]

@wolasss since you say that it happens irrespective of React version, I think it would make sense to create a new issue for it.

[CaptainN]

@denihs @StorytellerCZ When the latest release was made, did we make sure node_modules was deleted first?

[denihs]

Hi @CaptainN, yes. All I did was pull the release to my machine and run the publish command. The node_modules folder was never created.

[CaptainN]

That rules out this being a problem from packing in a copy of react. My guess it's something else.

[henriquealbert]

@wolasss since you say that it happens irrespective of React version, I think it would make sense to create a new issue for it.

I think it would make sense to create a new issue for it as well.

[StorytellerCZ]

Everything is working fine for me now, so I'm closing this issue.