gaearon / react-hot-loader

Tweak React components in real time. (Deprecated: use Fast Refresh instead.)
http://gaearon.github.io/react-hot-loader/
MIT License
12.26k stars 800 forks source link

Hooks unnecessarily update #1365

Open amcsi opened 4 years ago

amcsi commented 4 years ago

Whenever I make changes to a component that doesn't even involve hooks, all hooks with dependencies in them get updated undesirably. E.g.:

  const dispatch = useDispatch();
  useEffect(() => {
    dispatch(loadPosts());
  }, [dispatch]);

So any tiny text change I make anywhere, this useEffect() callback gets called, causing my spinner to show, and load my posts again unnecessarily.

If I removed dispatch from the hook dependencies, then the useEffect() does not get called, just what I want. But I violate the react-hooks/exhaustive-deps eslint rule that way.

Also, if I add any static value to the dependencies, then also, it always gets updated, which is not what I want. Note that each time I don't edit the hook itself, and in fact not even the same file as the hook.

Is this normal?

theKashey commented 4 years ago

Yes, unfortunately, this is how it works.

A first point here - https://github.com/gaearon/react-hot-loader/issues/1339, hot sure it would be ever fixed in RHL

amcsi commented 4 years ago

@theKashey Thanks. I'm glad to know that this is normal and not just me doing something wrong.

Something I noticed in the code by the way is this: https://github.com/gaearon/react-hot-loader/blob/c89e306985f2e819a8b933edaa085595ed95ebc1/src/reactHotLoader.js#L35-L47

There's something that checks if the reloadHooksOnBodyChange config options is true, then reloads the hook if the body changed. Sounds neat and good.

But then I read ahead, and there's that bit where it says if the hook has at least 1 dependency, then doesn't matter if the body didn't change, and doesn't even matter if the dependencies didn't changed: reloading of that hook happens, period.

Is there any reason why that is, and why the body and dependencies couldn't be checked there?

EDIT: by commenting out inputs.push(getHotGeneration());, hook reloading seems to be working the way I expect it to. Seems pretty logical to me on the surface, because now hooks depend on their regular dependencies and their function body contents. So if their dependencies change, they should rerun naturally, and also if their bodies change they should reload. I'll keep you posted.

EDIT 2: Like what @theKashey said below, the downside to this approach is that if a hook depends on functions/values from outside the hook, and even if the hooks only reloaded on their bodies or dependencies changing, they wouldn't be able to detect the outside values/functions (that are not dependencies) to automatically reload; thus a manual reload would be needed in those cases.

theKashey commented 4 years ago

Your hooks might use some other functions inside them, which might be changed, and that is not trackable using deps. So there are two possibilities:

amcsi commented 4 years ago

@theKashey yes, indeed that is what's happening. Thanks. I'll update my comment to reflect the downside of my approach. Hopefully this issue will help other people avoid wasting a lot of time trying to get hook hot reloading to work "better".

amcsi commented 4 years ago

@theKashey actually, what about if more dependencies would be added to the hook?

So this is how you'd normally write a useEffect to make the linter happy:

  useEffect(() => {
    dispatch(loadPosts());
  }, [dispatch]);

But this could possibly be done too:


  useEffect(() => {
    dispatch(loadPosts());
  }, [dispatch, loadPosts]);
```js

Normally if not using hot realoading, `loadPosts` would be redundant as a dependency since it never changes. However with hot reloading, it could actually change.
Maybe that would be a better guarantee for the hook reload to happen when we really want it to. There could also be a stricter version of exhaustive-deps made for this.
theKashey commented 4 years ago

Well, React-Fast-Refresh, an upcoming replacement for React-Hot-Loader, would update hooks even without any dependencies. However, not all, but only in locations, "affected" by a change (read even more unpredictable).

This is why "not sure it would be ever fixed in RHL" - there is no much sense to spend time fixing it.