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

Bug: Empty deps useEffect be called on re-render in HMR #21019

Closed
xialvjun opened this issue Mar 16, 2021 · 5 comments
Closed

Bug: Empty deps useEffect be called on re-render in HMR #21019

xialvjun opened this issue Mar 16, 2021 · 5 comments
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@xialvjun
Copy link

Empty dependencies useEffect should be called only one time.

React version: v17.0.1
react-refresh: v0.9.0

Steps To Reproduce

Link to code example:

https://codesandbox.io/s/headless-bash-0fhis?file=/src/App.js

The current behavior

Re-render in HMR cause empty deps useEffect rerun.

The expected behavior

Empty deps useEffect should run only one time, and not rerun on re-render in HMR.

@xialvjun xialvjun added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Mar 16, 2021
@gaearon
Copy link
Collaborator

gaearon commented Mar 16, 2021

This is not a bug, it's intentional behavior and the only way this feature could work.

When we update the code, we need to "clean up" the effects that hold onto past values (e.g. passed functions), and "setup" the new ones with updated values. Otherwise, the values used by your effect would be stale and "disagree" with value used in your rendering, which makes Fast Refresh much less useful and hurts the ability to have it work with chains of custom Hooks.

Your effects should generally be written in a way that cleaning it up + re-running it does not cause problems. This is not just important for Fast Refresh, but for other forward-looking features like background rendering or row virtualization where effects will also cleanup on "hide" and setup on "show". Effects are not exactly "mount"/"unmount" — they're more like "show"/"hide".

Hope this helps!

@gaearon gaearon closed this as completed Mar 16, 2021
@xialvjun
Copy link
Author

xialvjun commented Mar 17, 2021

But we have declared that the effect depends on no other things.

In your words, I try to think: state is just as a mechanical spring, and effect is as a force.

But does it really meet our needs? Is it really the word effect's meaning?

In current status, it seems every effect needs clean up. But didn't we treat effect as a tool to communicate with external system? How can we clean up effect if the external system doesn't support to undo its operations.

When we update the code, we need to "clean up" the effects that hold onto past values (e.g. passed functions), and "setup" the new ones with updated values.

In my opinion, since we have declared that the effect depends on no other things, react-refresh needn't to worry about the effect become stale.

@gaearon
Copy link
Collaborator

gaearon commented Mar 17, 2021

Technically effect always depends on the code of the function itself. Normally it can’t change. But Fast Refresh presents a situation in which conceptually the code itself “changes”. So it’s like an implicit dependency.

How can we clean up effect if the external system doesn't support to undo its operations.

Usually in that case the solution is “reinstatiating” that system. For example if it only exposes a destroy method, then you need to make sure that the effect itself creates it.

@gaearon
Copy link
Collaborator

gaearon commented Mar 17, 2021

If you have a concrete case as a sandbox I can look in more detail.

@xialvjun
Copy link
Author

Well, I think I get it.

HMR is the special case in which the effect code may change.
To keep the effect sync with the code, react-refresh will rerun the empty deps effect.
In normal js running, the effect code will not change, it's static, so everything is ok.

Thanks for your explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

2 participants