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

Add support for effects with cleanup? #64

Closed
mindplay-dk opened this issue Oct 10, 2019 · 13 comments
Closed

Add support for effects with cleanup? #64

mindplay-dk opened this issue Oct 10, 2019 · 13 comments

Comments

@mindplay-dk
Copy link
Contributor

Should we add effects with cleanup?

There is currently no way to perform an action when a component gets destroyed.

@yisar
Copy link
Collaborator

yisar commented Oct 10, 2019

useEffect(() => {
    function handleStatusChange(status) {
      setIsOnline(status.isOnline)
    }

    ChatAPI.subscribeToFriendStatus(props.friend.id, handleStatusChange)

    return function cleanup() {
      ChatAPI.unsubscribeFromFriendStatus(props.friend.id, handleStatusChange)
    }
}

The use case from the react document looks like when useEffect returns a function, we execute it
The function contains unmouted logic
Am I correct?

@mindplay-dk
Copy link
Contributor Author

Yes:

the function passed to useEffect may return a clean-up function [...] The clean-up function runs before the component is removed from the UI to prevent memory leaks. Additionally, if a component renders multiple times (as they typically do), the previous effect is cleaned up before executing the next effect

@mindplay-dk
Copy link
Contributor Author

(Preact hooks are a good source of reference, as the code is very simple and easy to understand.)

@yisar
Copy link
Collaborator

yisar commented Oct 10, 2019

Well, I need to think carefully about the correct implementation here. My previous understanding may be insufficient.~

yisar added a commit that referenced this issue Oct 10, 2019
@yisar
Copy link
Collaborator

yisar commented Oct 10, 2019

@mindplay-dk I refactored the useEffect, and now it should be able to resolve these two issues #63 #64

@yisar
Copy link
Collaborator

yisar commented Oct 10, 2019

supported Now!

@mindplay-dk
Copy link
Contributor Author

Looks good, just one small deviation from how React works - from the description in the yellow note box on this page:

If you want to run an effect and clean it up only once (on mount and unmount), you can pass an empty array ([]) as a second argument. This tells React that your effect doesn’t depend on any values from props or state, so it never needs to re-run.

So, to summarize:

useEffect(f)       // ✅ effect (and clean-up) every time
useEffect(f, [])   // ❌ effect (and clean-up) only once in a component's life
useEffect(f, [x])  // ✅ effect (and clean-up) when property x changes

@yisar
Copy link
Collaborator

yisar commented Oct 10, 2019

Where is the deviation? The summarize is right 😆

@yisar
Copy link
Collaborator

yisar commented Oct 10, 2019

@mindplay-dk I tried the use case of react, which would not execute when the array was empty.

useEffect(f, [])   // ✅ return function( such as clean up) should not run.

So fre's current behavior should be consistent with react

@mindplay-dk
Copy link
Contributor Author

I tried the use case of react, which would not execute when the array was empty.

The clean-up function of an effect always runs - the question is only when.

With an empty array, the clean-up function should run only once, when the component is destroyed.

yisar added a commit that referenced this issue Oct 10, 2019
@yisar
Copy link
Collaborator

yisar commented Oct 10, 2019

I didn't even notice this detail. I've fixed it and it works now 💯

@mindplay-dk
Copy link
Contributor Author

Looks good!

Still needs tests for useEffect() with no second argument, and with [] - let's leave this issue open until all the tests are in place.

@mindplay-dk
Copy link
Contributor Author

We have tests for this now. Closing!

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