Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hooks unnecessarily update #1365

Open
amcsi opened this issue Oct 22, 2019 · 6 comments
Open

Hooks unnecessarily update #1365

amcsi opened this issue Oct 22, 2019 · 6 comments

Comments

@amcsi
Copy link

amcsi commented Oct 22, 2019

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
Copy link
Collaborator

Yes, unfortunately, this is how it works.

A first point here - #1339, hot sure it would be ever fixed in RHL

@amcsi
Copy link
Author

amcsi commented Oct 22, 2019

@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:

// reload hooks which have changed string representation
if (configuration.reloadHooksOnBodyChange) {
inputs.push(String(cb));
}
if (
// reload hooks with dependencies
deps.length > 0 ||
// reload all hooks of option is set
(configuration.reloadLifeCycleHooks && deps.length === 0)
) {
inputs.push(getHotGeneration());
}

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
Copy link
Collaborator

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:

  • update only what changed, and probably not update everything that has to be updated 😭
  • update everything 👍

@amcsi
Copy link
Author

amcsi commented Oct 23, 2019

@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 amcsi closed this as completed Oct 23, 2019
@amcsi
Copy link
Author

amcsi commented Oct 23, 2019

@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.

@amcsi amcsi reopened this Oct 23, 2019
@theKashey
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants