Safe versions of react hooks

[urugator]

useEffect(() => mobxStore.a, [mobxStore]) is an error, because mobxStore isn't immutable, but eslint-plugin-react-hooks doesn't know and therefore doesn't warn.
useMemo(() => mobxStore.a, [mobxStore]) callback runs within observer, therefore component subcribes for mobxStore.a, but a change of mobxStore.a only causes re-render without updating the memoized value.

Idea 1

Expose our own version of these hooks that check you do not pass observable as depenency:

function useEffect(cb, deps) {
  if (process.env.NODE_ENV !== "production" && deps) {
    for (const dep of deps) {
      if (isObservable(dep)) {
        console.warn("Do not pass observables to deps, but individual primitives, eg. `[store.number, store.fetch]` instead of `[store]`")
      }
    }      
  }
  return React.useEffect(cb, deps);
}

Idea 2

Expose our own version of these hooks that are reactive:

function useEffect(cb, deps) {  
  if (!deps) return React.useEffect(cb);
  const [state, setState] = React.useState(); // opt: reuse current observer's forceUpdate
  return React.useEffect(() => {
    // Note we simply invalidate the component, we don't run cb as reaction's effect
    const reaction = new Reaction('useEffect', () => setState(Symbol()));
    reaction.track(cb)
    return () => reaction.dispose()
  }, [state, ...deps]); // opt: use own comparison to avoid copy
}

This is more convinient, but has a runtime cost.

Keep in mind the idea is not to avoid the need to pass dependencies or to provide some optimization benefits. The idea is just to make sure mobx plays well with eslint-plugin-react-hooks and the way the hooks are designed.

In both cases we can introduce eslint rule, that forbids React.useFoo and import { useFoo } from React.
EDIT: I think we can limit this to uses inside observer (since we can assume there are no observables without observer)

[kubk]

Regarding the first idea. A common pattern is to fetch data on component mount like this:

const Component = observer(() => {
  const { userStore } = useStore();
  
  useEffect(() => {
    userStore.fetch();
  }, [userStore]); // pass argument to avoid react eslint warning
})

Since isObservable(userStore) returns true it won't be possible to use this pattern anymore. How should I fetch data when the rule is enabled?

The idea is just to make sure mobx plays well with eslint-plugin-react-hooks and the way the hooks are designed.

In my opinion passing store to useEffect arguments is OK. useEffect re-runs on every argument reference change, as a developer you can assume that useEffect(() => {}, []) and useEffect(() => {}, [someStore]) are essentially not different from each other.

I agree that passing an observable value to useEffect is wrong. For example useEffect(() => {}, [store.arrayProperty]). Is there a way to distinguish stores from store properties?

[urugator]

Since isObservable(userStore) returns true it won't be possible to use this pattern anymore.

Which is correct, because there is no guarantee that fetch is stable (only you know). eslint-plugin-react-hooks assumes the userStore is immutable and therefore fetch can't change unless userStore changes as well. The correct thing to do is to pass the function directly:

const { fetch } = userStore;
useEffect(() => {
  fetch()
}, [fetch]);

In my opinion passing store to useEffect arguments is OK.

It has exactly the same effect as supressing eslint-plugin-react-hooks. Consider:

useEffect(() => {
 console.log(userStore.name)
}, [userStore]);

// Following is functionally equivalent (it will log only on mount)
// but `eslint-plugin-react-hooks` will shout at you, that you're missing the `name` in deps
const [name, setName] = useState("default");
useEffect(() => {
 console.log(name)
}, []);

// And if you fix the problem as suggested by  `eslint-plugin-react-hooks`
// you get something that is functionally different
// because it runs on every `name` change

So the user is mislead into thinking he fixed the problem reported by eslint-plugin-react-hooks, but all he did is de-facto /* @eslint-disable */

EDIT: moved to reaction

[kubk]

@urugator I do agree that providing store as argument to useEffect is basically the same as ignoring the ESlint rule. It's because the immutable model of React is different from mutable model of Mobx. So far I am satisfied with adding store to useEffect arguments because it's simple and straightforward. Let's wait for more feedback.

[urugator]

So far I am satisfied with adding store to useEffect arguments because it's simple and straightforward

It's only simple and straightforward as long as your intention is to do something completely unidiomatic.
The default useEffect behavior is to run on every render.
You can optimize it to run only when anything accessed changes. eslint-plugin-react-hooks would normally help with that, but not here.
In very rare edge cases you may need to intentionally omit a dependency and in such case you have to /* @eslint-disable eslint-plugin-react-hooks */.

So either you're saying that whenever you need to access store in useEffect it's the third case and you're consciously omitting dependency (unlikely) or you're aware that eslint-plugin-react-hooks can't help you here and therefore you're extra cautious when accessing observables in effect (far from simple and straightforward imo).

I do agree it's cumbersome, as I said the idea 2 feels more natural in this regard. Thank you for the feedback btw.

[urugator]

Btw both ideas play nice with makeAutoObservable/observable + observableRequiresReaction. Currently following throws

const store = makeAutoObservable({ action() {} });
configure({ observableRequiresReaction: true })
useEffect(() => {
  store.action();  
}, [store])

because store.action is observable. It won't throw for idea 2, because there is a reaction, for idea 1, we can wrap the effect in action.

On the other hand, this wouldn't work correctly for idea 2

useObserverEffect(/*here*/action(() => {
  store.action();
}), [store])

because action applies untracked. We can warn for this specific case, but user can still introduce runInAction or pass observables to action as params and access them there (technically they're still effect's dependencies...). This would be quite rare I suppose, you can get the same problem with autorun/reaction. Which brings me to question, why action actually applies untracked, when is it helpful?

[konstantin-tarasov-clearscale]

We definitely shouldn't hit the performance with idea 2.
I guess, additional eslint rule mobx-hooks-rules implementing idea 1 with warning will be enough.

[urugator]

We definitely shouldn't hit the performance with idea 2.

Pretty resolute :) Just want to point out both of these options are completely optional and one could even pick one or another based on preference. Obviously it would be ideal to have a single recommendation, especially for newcomers.

Regarding performance, it's not necessarily worse: The overhead is creation/disposal of the reaction. For effects that don't trigger very often it shouldn't matter much. Actually with a larger number of (observable) deps, you should get better perf, because react doesn't have to check these dependencies every render, just to find out nothing changed.
For effects that run rapidly, like every render, it's usually better to not provide deps list at all - in this case we obviously don't create the reaction.

I guess, additional eslint rule mobx-hooks-rules implementing idea 1 with warning will be enough.

It's not statically analyzable, it can't be implemented just as eslint plugin.

[urugator]

Another possible problem with idea 2 are async calls:

useObserverEffect(() => {
  fetch('')
  .then(() => {
    // Untracked, but the result potentially depends on this
    console.log(obj.x);
  })
}, [obj])

I don't really like this and I don't see a way around that.