fix(useStorageValue): Allow setting of state even if the component has not mounted #959

ArttuOll · 2022-10-08T13:39:14Z

What is the current behavior, and the steps to reproduce the issue?

The current behavior has been described in #451. In short, using the setValue function provided by useSessionStorageValue and useLocalStorageValue on first mount could cause other instances of those hooks to get out of sync.

What is the expected behavior?

Every instance of the useSessionStorageValue and useLocalStorageValue hooks should return the same value at all times.

How does this PR fix the problem?

By using useState to store the value of the tracked key instead of useSafeState.

In the sandbox below, I have modified the useSessionValue hook in the above way. The example is the one provided in #451 , but this time the reported issue is not present.

Why does this work, you might ask? I do not know. I will have to digest this a bit more.

I arrived at this solution from the simple notion that the state variable is what gets out of sync. I then started going through everything that affected state. I went through many things, but finally I noticed that useSafeState also "affects" state (in a sense that it is not the standard useState that I picture in my mind when I think about React state). I then tried switching it to
a traditional useState, which seemed to solve the issue.

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?
- useSessionStorageValue same key, different value #451
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?

codecov · 2022-10-08T13:42:33Z

Codecov Report

Merging #959 (28e73ad) into master (8411792) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #959   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           60        60           
  Lines         1060      1059    -1     
  Branches       184       184           
=========================================
- Hits          1060      1059    -1

Impacted Files	Coverage Δ
src/useStorageValue/useStorageValue.ts	`100.00% <100.00%> (ø)`

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

ArttuOll · 2022-10-09T07:39:04Z

Why does this work, you might ask? I do not know. I will have to digest this a bit more.

Maybe what happens is something like this (referring to the components in my codesandbox):

ValueDisplay mounts and displays the current value in the storage.
Provider mounts, changing the value in the storage.
ValueDisplay has already mounted, so useSafeState allows its state to update.
The components after Provider have not yet mounted and thus useSafeState does not allow their states to be updated / scheduled to be updated / something like this, I know this does not make any React sense. When they mount, they display the value which their useSessionStorage hooks had when they were initialized.

Just a quick thought.

Ps. that codecov/patch action certainly seems to take its time 😄

…s not mounted Using normal useState to store the value of the tracked key instead of useSafeState seems to solve the problem described in #451. fix #451

xobotyi · 2022-10-09T13:07:29Z

Well, okay, the reason - i've finally got it =) StrictMode + useFirstMountState + useIsMounted - sandbox is using strict mode, if to disable it - issue not presented.

What we know about strict mode - effects are triggered twice, useFirstMountState and useIsMounted unable to work wtithin strict mode, since it is based off useRef.

ArttuOll · 2022-10-09T13:13:56Z

Oh, nice! I should have thought about StrictMode 😆 It's so easy to forget, but should be taken into account when debugging...

ArttuOll · 2022-10-09T13:16:08Z

But what should be done about this? This bug is very distracting, even if it only exists in StrictMode, ie. during development.

xobotyi · 2022-10-09T13:18:04Z

I'm trying to make a solution at the moment, but i see literally no chance of making useIsMounted survivable to doubled effects

xobotyi · 2022-10-09T13:19:25Z

Ofc we can go with solution you proposed, but it is only fix of single symptom, not its source.

ArttuOll · 2022-10-09T13:27:03Z

Well, this relates to another thing I had in mind earlier today. Why are we using useSafeState in the first place?

According to the React team it is an anti-pattern to check whether the component is mounted or not before setting state. They argue that any state setting functions should be cancelled before the component unmounts instead. They go more detail into this in the blog post. This article talks about the same topic, but is a bit more fresh.

ArttuOll · 2022-10-09T13:41:11Z

To expand on my previous comment:

From the perspective of a hook library, it essentially should be the user of the stateful hook's responsibility to check that the state is not set after the component has unmounted, not the library's.

xobotyi · 2022-10-09T13:44:59Z

Mostly useSafeState used within hooks that are making use of any WEB API or has event handlers, where any event may occur during component unmounts.

ArttuOll · 2022-10-09T14:00:19Z

Mostly useSafeState used within hooks that are making use of any WEB API or has event handlers, where any event may occur during component unmounts.

Indeed. And the articles I linked both argue that in such situations, it would be correct to cancel the state settings functions (maybe an async event handler or a useEffect making a web request) when the component unmounts, rather than checking for the mounted state before settings state.

As pointed in the second of the articles I referenced, useSafeState merely hides the Can’t perform a React state update on an unmounted component warning. React already does the isMounted check before updating state and gives that warning, if a component is not mounted when a state update is attempted. Doing any isMounted checks application-side merely move the check from React internals to the application, hiding the warning.

The React team also makes a good point here:

Other uses of isMounted() are similarly erroneous; using isMounted() is a code smell because the only reason you would check is because you think you might be holding a reference after the component has unmounted.

xobotyi · 2022-10-09T14:05:48Z

it is impossible to cancel async chain midair, in case processing went further direct handlers - you're stuck
im taling about

Promise.
then(()=>{
 Promise2ThatTakesLotsOfTime().then(setState) // this one already went async
 // unmount happens here
})

As for now there is an AbortToken, but its not so widely common

ArttuOll · 2022-10-09T14:06:21Z

Mostly useSafeState used within hooks that are making use of any WEB API or has event handlers, where any event may occur during component unmounts.

And to respond in the context of this library, useSafeState is also heavily used internally, which appears unnecessary to me, and it also seems to have caused the bug this PR solves (in a way).

ArttuOll · 2022-10-09T14:11:00Z

it is impossible to cancel async chain midair, in case processing went further direct handlers - you're stuck im taling about
Promise.
then(()=>{
 Promise2ThatTakesLotsOfTime().then(setState)
 // unmount happens here
})

Isn't this possible using the AbortController API, maybe in combination with useEffect?

From the second article

useEffect(() => {
  const abortController = new AbortController()   // creating an AbortController
  fetch(url, { signal: abortController.signal })  // passing the signal to the query
    .then(data => {
      setState(data)                              // if everything went well, set the state
    })
    .catch(error => {
      if (error.name === 'AbortError') return     // if the query has been aborted, do nothing
      throw error
    })
  
  return () => {
    abortController.abort()                       // stop the query by aborting on the AbortController on unmount
  }
}, [])

In any case, in the article the React team seems to suggest that such state setting functions should be made cancellable.

xobotyi · 2022-10-09T14:11:40Z

TBC im sitting with a thought of dropping lots of useSafeState usages for a month or so.
React18 anyway dropped the message about setting state on unmounted component.

ArttuOll · 2022-10-09T14:16:57Z

I'm glad to hear that! Let me know, if I can help in refactoring away the useSafeState's.

xobotyi · 2022-10-09T14:17:07Z

As said - yes, abort controller kinda solves situation, but it is not widely supported.

and frankly speaking its usage quite ugly, only try\catch blocks are uglier.

ArttuOll · 2022-10-09T14:21:55Z

As said - yes, abort controller kinda solves situation, but it is not widely supported.

and frankly speaking its usage quite ugly, only try\catch blocks are uglier.

AbortController not having wide support is a fair point. Plain promises surely can be cancelled somehow by (ab)using Promise.reject but I don't even want to imagine how tedious that would be. That would be appear to be necessary, however.

Also, using AbortController can get pretty ugly, but useAsyncAbortable makes it a lot prettier 😉 .

xobotyi · 2022-10-11T08:51:17Z

welp, everything ended up with complete useStorageValue rework :D

ArttuOll · 2022-10-11T08:58:12Z

That is probably good, the code felt quite difficult before 😄

I got kind of inspired by our conversation here. What do you think about this:

I open an issue with a complete write up of our discussion here so that everybody can easily see it and take part.
I make a PR with all the internal usages of useSafeState removed.

ArttuOll · 2022-10-11T09:39:19Z

Also, I noticed that you used this PR's solution for the bug as part of #960 . Do you mind adding the hacktoberfest-accepted label to this PR?

xobotyi · 2022-10-12T13:51:39Z

There should be another label for that? i thought just hascktoberfest would be fine

xobotyi · 2022-10-12T13:53:54Z

@ArttuOll, are you planning to continue work on react-hookz or is it just hacktoberfest related?

ArttuOll · 2022-10-12T14:58:52Z

There should be another label for that? i thought just hascktoberfest would be fine

Yeah, if this PR is not merged, but the code is accepted, that's how they know to count this to my score.

Will you be working on this after October

Yes, so no worries that I would just drop everything that I'm working on when this month ends 😁 perhaps not as actively

xobotyi · 2022-10-12T16:15:32Z

@ArttuOll what do you think joining team and becoming maintainer?

ArttuOll · 2022-10-12T17:58:53Z

Well, I'm pretty sure I would be able to find time for that and that would surely be interesting, so count me in!

xobotyi · 2022-11-03T13:11:06Z

🎉 This issue has been resolved in version 17.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

ArttuOll marked this pull request as ready for review October 8, 2022 14:07

fix(useStorageValue): Allow setting of state even if the component ha…

28e73ad

…s not mounted Using normal useState to store the value of the tracked key instead of useSafeState seems to solve the problem described in #451. fix #451

ArttuOll force-pushed the pr/fix-451 branch from acd45da to 28e73ad Compare October 9, 2022 12:02

xobotyi mentioned this pull request Oct 11, 2022

Rework useStorageValue to more simple and robust variant #960

Merged

xobotyi added the hacktoberfest-accepted label Oct 12, 2022

xobotyi closed this in #960 Nov 3, 2022

xobotyi added the released label Nov 3, 2022

ArttuOll mentioned this pull request Dec 20, 2022

Migrate usePromise from react-use #1048

Closed

7 tasks

ArttuOll mentioned this pull request Jan 23, 2023

Deprecate useSafeState #1096

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(useStorageValue): Allow setting of state even if the component has not mounted #959

fix(useStorageValue): Allow setting of state even if the component has not mounted #959

ArttuOll commented Oct 8, 2022 •

edited

Loading

codecov bot commented Oct 8, 2022 •

edited

Loading

ArttuOll commented Oct 9, 2022 •

edited

Loading

xobotyi commented Oct 9, 2022 •

edited

Loading

ArttuOll commented Oct 9, 2022

ArttuOll commented Oct 9, 2022

xobotyi commented Oct 9, 2022

xobotyi commented Oct 9, 2022 •

edited

Loading

ArttuOll commented Oct 9, 2022

ArttuOll commented Oct 9, 2022

xobotyi commented Oct 9, 2022

ArttuOll commented Oct 9, 2022 •

edited

Loading

xobotyi commented Oct 9, 2022 •

edited

Loading

ArttuOll commented Oct 9, 2022

ArttuOll commented Oct 9, 2022

xobotyi commented Oct 9, 2022

ArttuOll commented Oct 9, 2022

xobotyi commented Oct 9, 2022

ArttuOll commented Oct 9, 2022 •

edited

Loading

xobotyi commented Oct 11, 2022

ArttuOll commented Oct 11, 2022

ArttuOll commented Oct 11, 2022

xobotyi commented Oct 12, 2022

xobotyi commented Oct 12, 2022

ArttuOll commented Oct 12, 2022

xobotyi commented Oct 12, 2022

ArttuOll commented Oct 12, 2022

xobotyi commented Nov 3, 2022

fix(useStorageValue): Allow setting of state even if the component has not mounted #959

fix(useStorageValue): Allow setting of state even if the component has not mounted #959

Conversation

ArttuOll commented Oct 8, 2022 • edited Loading

What is the current behavior, and the steps to reproduce the issue?

What is the expected behavior?

How does this PR fix the problem?

Checklist

codecov bot commented Oct 8, 2022 • edited Loading

Codecov Report

ArttuOll commented Oct 9, 2022 • edited Loading

xobotyi commented Oct 9, 2022 • edited Loading

ArttuOll commented Oct 9, 2022

ArttuOll commented Oct 9, 2022

xobotyi commented Oct 9, 2022

xobotyi commented Oct 9, 2022 • edited Loading

ArttuOll commented Oct 9, 2022

ArttuOll commented Oct 9, 2022

xobotyi commented Oct 9, 2022

ArttuOll commented Oct 9, 2022 • edited Loading

xobotyi commented Oct 9, 2022 • edited Loading

ArttuOll commented Oct 9, 2022

ArttuOll commented Oct 9, 2022

xobotyi commented Oct 9, 2022

ArttuOll commented Oct 9, 2022

xobotyi commented Oct 9, 2022

ArttuOll commented Oct 9, 2022 • edited Loading

xobotyi commented Oct 11, 2022

ArttuOll commented Oct 11, 2022

ArttuOll commented Oct 11, 2022

xobotyi commented Oct 12, 2022

xobotyi commented Oct 12, 2022

ArttuOll commented Oct 12, 2022

xobotyi commented Oct 12, 2022

ArttuOll commented Oct 12, 2022

xobotyi commented Nov 3, 2022

ArttuOll commented Oct 8, 2022 •

edited

Loading

codecov bot commented Oct 8, 2022 •

edited

Loading

ArttuOll commented Oct 9, 2022 •

edited

Loading

xobotyi commented Oct 9, 2022 •

edited

Loading

xobotyi commented Oct 9, 2022 •

edited

Loading

ArttuOll commented Oct 9, 2022 •

edited

Loading

xobotyi commented Oct 9, 2022 •

edited

Loading

ArttuOll commented Oct 9, 2022 •

edited

Loading