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

Deprecate useSafeState #1096

Merged
merged 2 commits into from
Mar 23, 2023
Merged

Deprecate useSafeState #1096

merged 2 commits into from
Mar 23, 2023

Conversation

ArttuOll
Copy link
Contributor

@ArttuOll ArttuOll commented Jan 23, 2023

What is the current behavior?

useSafeState is offered as a solution to situations where asynchronous functions attempt to update a component's state when the component has already unmounted, resulting in the famous Can't perform a React state update on an unmounted component error.

useSafeSate is like useState, but it does an isMounted check before allowing the state to be set. This, according to the React team, is an anti-pattern. A more fresh take on the topic, confirmed by Dan Abramov, can be found here. I won't go in to the details of why isMounted checks are an anti-pattern, because I don't want to repeat the resources linked above.

Additionally, the Can't perform a React state update on an unmounted component error has been removed in React 18, because it was considered to be misleading and caused people to invent workarounds, which the React team considered harmful (the example of a harmful workaround in the linked discussion is actually almost exactly useIsMounted).

For the above reasons, I think useSafeState has no job anymore. Lastly, it should also be considered, whether or not useIsMounted should be removed also.

This issue was also discussed a while back in #959 .

What is the expected behavior?

useState should be used instead of useSafeState. Asynchronous functions should be cancelled if the component unmounts or they should be ignored, if cancellation is not an option. From the author of the discussion linked above:

... We’re removing the warning because people work around it with isMounted which defeats the purpose. So we might as well not warn. But the proper solution is cancellation. Or ignoring if you’re lazy about cancelling.

How does this PR fix the problem?

  1. Replace all internal usages of useSafeState with useState.
  2. Remove useSafeState.
  3. Remove any mentions of useSafeState from the documentation and examples.

Checklist

  • Have you read contribution guideline?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Is there an existing issue for this PR?
    • link issue here
  • Have the files been linted and formatted?
  • Have the docs been updated to match the changes in the PR?
  • Have the tests been updated to match the changes in the PR?
  • Have you run the tests locally to confirm they pass?

useSafeState is an anti-pattern (detailed more in the associated PR) which merely hides a React's
warning. As of React 18, this warning is not even shown anymore, which makes this hook even more
unnecessary.

BREAKING CHANGE: useSafeState is removed
@ArttuOll ArttuOll requested a review from a team January 23, 2023 13:09
@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Merging #1096 (01ece87) into master (f6829c0) will decrease coverage by 0.03%.
The diff coverage is 96.66%.

@@            Coverage Diff             @@
##           master    #1096      +/-   ##
==========================================
- Coverage   98.52%   98.49%   -0.03%     
==========================================
  Files          63       62       -1     
  Lines        1082     1064      -18     
  Branches      180      179       -1     
==========================================
- Hits         1066     1048      -18     
  Misses          2        2              
  Partials       14       14              
Impacted Files Coverage Δ
src/index.ts 100.00% <ø> (ø)
src/useNetworkState/index.ts 73.07% <50.00%> (-1.00%) ⬇️
src/useAsync/index.ts 100.00% <100.00%> (ø)
src/useCookieValue/index.ts 100.00% <100.00%> (ø)
src/useDebouncedState/index.ts 100.00% <100.00%> (ø)
src/useFunctionalState/index.ts 100.00% <100.00%> (ø)
src/useIntersectionObserver/index.ts 98.18% <100.00%> (-0.04%) ⬇️
src/useMeasure/index.ts 100.00% <100.00%> (ø)
src/useMediatedState/index.ts 90.90% <100.00%> (-0.76%) ⬇️
src/usePermission/index.ts 100.00% <100.00%> (ø)
... and 5 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@xobotyi xobotyi left a comment

Choose a reason for hiding this comment

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

Thanks for thorough explanation.

We will let it to marinate a bit more to avoid overwhelming with breaking changes (supposedly we should have a new rule - one breaking change in a month 😅)

@ArttuOll
Copy link
Contributor Author

ArttuOll commented Feb 5, 2023

What do you think about also removing useIsMounted? I think the same reasoning applies to it as well. I could add it to this PR.

@xobotyi
Copy link
Contributor

xobotyi commented Feb 5, 2023

That one still have valid usecases, when dealing with external queues, high frequency events etc.

@ArttuOll
Copy link
Contributor Author

Hello @xobotyi , it's been a while. Do you think it would be time to merge this?

@xobotyi
Copy link
Contributor

xobotyi commented Mar 23, 2023

yup

@xobotyi xobotyi merged commit 284e499 into master Mar 23, 2023
github-actions bot pushed a commit that referenced this pull request Mar 23, 2023
# [23.0.0](v22.0.0...v23.0.0) (2023-03-23)

* Deprecate useSafeState (#1096) ([284e499](284e499)), closes [#1096](#1096)

### BREAKING CHANGES

* `useSafeState` hook is removed
@xobotyi
Copy link
Contributor

xobotyi commented Mar 23, 2023

🎉 This PR is included in version 23.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants