Skip to content
This repository has been archived by the owner on Dec 31, 2020. It is now read-only.

fix for useDisposable, update some dev deps #46

Merged
merged 8 commits into from
Jan 19, 2019
Merged

Conversation

xaviergonz
Copy link
Contributor

@xaviergonz xaviergonz commented Jan 17, 2019

According to an update of the react hooks faq:

https://reactjs.org/docs/hooks-faq.html#how-to-memoize-calculations

You may rely on useMemo as a performance optimization, not as a semantic guarantee. In the future, React may choose to “forget” some previously memoized values and recalculate them on next render, e.g. to free memory for offscreen components. Write your code so that it still works without useMemo — and then add it to optimize performance. (For rare cases when a value must never be recomputed, you can lazily initialize a ref.)

This PR uses the recommended approach of lazy initialization, plus makes sure the effect is not re-created when some inputs are given and the effect is manually early disposed

Also updates some dev deps

@xaviergonz xaviergonz requested a review from danielkcz January 17, 2019 19:45
@xaviergonz
Copy link
Contributor Author

Btw, I switched useUnmount to useLayoutEffect since it fixed an issue with useDisposable and basically it is being used internally to dispose a reaction (so the disposal doesn't get delayed), just to be on the safe side.

@coveralls
Copy link

coveralls commented Jan 17, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 3dfd822 on fix-useDisposable into 5f7d9ca on master.

Copy link
Collaborator

@danielkcz danielkcz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some small nit-picks, good work otherwise for sure! Thanks.

src/useDisposable.ts Outdated Show resolved Hide resolved
src/useDisposable.ts Outdated Show resolved Hide resolved
src/useDisposable.ts Outdated Show resolved Hide resolved
src/useDisposable.ts Outdated Show resolved Hide resolved
src/useDisposable.ts Outdated Show resolved Hide resolved
test/utils.ts Outdated Show resolved Hide resolved
test/useObservableEffect.test.tsx Outdated Show resolved Hide resolved
src/useDisposable.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@danielkcz danielkcz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last small thing. Also, coverage has dropped by 2% because of that production check. Nothing too horrible, but should be fairly easy to fix. Just set a NODE_ENV for the test and revert it after.

src/useDisposable.ts Outdated Show resolved Hide resolved
@xaviergonz
Copy link
Contributor Author

done, checks for prod mode and console error an actual error object

@danielkcz danielkcz merged commit 1f900f2 into master Jan 19, 2019
@danielkcz danielkcz deleted the fix-useDisposable branch January 19, 2019 11:19
@danielkcz
Copy link
Collaborator

Thank you very much for the work.

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

Successfully merging this pull request may close these issues.

3 participants