open-amdocs / webrix

Powerful building blocks for React-based web applications
https://webrix.amdocs.com
Apache License 2.0
431 stars 31 forks source link

fix(#77): Removed useUnmounted hook and cleanup of useMounted #78

Closed gaeaehrlich closed 2 years ago

gaeaehrlich commented 2 years ago

The useUnmounted hook does not work. Here's why: When the cleanup function of the useEffect runs, and the value of the reference is being changed to true, it doesn't actually reflect to the component using the hook. The reason for this is that the return statement of the hook is not being called after the unmount. This hook should be implemented anew for each use case, and can not be implemented as a custom hook. For the same reason, removed cleanup function in useEffect of useMounted.

codecov[bot] commented 2 years ago

Codecov Report

Merging #78 (6be12c0) into master (f4191e7) will decrease coverage by 0.02%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
- Coverage   96.00%   95.98%   -0.03%     
==========================================
  Files          61       61              
  Lines         852      847       -5     
  Branches       29       29              
==========================================
- Hits          818      813       -5     
  Misses         33       33              
  Partials        1        1              
Impacted Files Coverage Δ
src/hooks/useMounted/useMounted.js 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f4191e7...6be12c0. Read the comment docs.

ykadosh commented 2 years ago

@gaeaehrlich @yairEO This is a breaking change, so you cannot use fix() here, it has to be perf(), and note that this will release a major version (2.0.0), so usually it's best to prepare for such a release by introducing all the breaking changes together (there are a few breaking suggestions under issues).

yairEO commented 2 years ago

Closing this one because an identical PR was merged into 2.x branch