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

Extending useState result with getState #21220

Closed
YuriGor opened this issue Apr 9, 2021 · 8 comments
Closed

Extending useState result with getState #21220

YuriGor opened this issue Apr 9, 2021 · 8 comments

Comments

@YuriGor
Copy link

YuriGor commented Apr 9, 2021

Hi, it's an often case when we cannot rely on current state value given by useState, e.g. if we need this current value inside some function - it will be 'stale'.
One of the ways to really get latest state is to call setState method given by useState and pass callback as an argument to the setState, so this callback will receive up to date state value.
For myself I created a wrapper for useState, which is giving third result array element, and this element is getState function:

import { useState } from 'react';

export default function useGetState(defaultState) {
  const [state, setState] = useState(defaultState);
  return [
    state,
    setState,
    () => {
      let value;
      setState((old) => {
        value = old;
        return old;
      });
      return value;
    },
  ];
}

usage example:

const [state, setState, getState] = useGetState(111);
function(){
   const latestState = getState();
}

As you see this getState function is just non-mutating call of setState just to grab the fresh state value.

It would be great to extend the original useState hook in a similar, but, maybe, a bit more efficient way,
because I believe original setState makes some extra stuff not needed if we only need to get state and nothing more.

@YuriGor YuriGor changed the title extending useState result with getState Extending useState result with getState Apr 9, 2021
@jacty
Copy link

jacty commented Apr 9, 2021

I am sorry, I cannot get the point to getState(). Since state and setState() should only be used in and between Components in Render Phase and if state is not latest which means the latest render hasn't been run yet and that state should not been deemed as reliable, I am afraid.

@YuriGor
Copy link
Author

YuriGor commented Apr 9, 2021

@jacty
I my case this mostly happens with event handler functions, if you need to use some state in handler and try to use state value directly - you will have old value version captured at the moment of handler creation (that one closure of the render method call remains around handler as a closure)
Here is some article from internets describing this case maybe better then me.
https://dmitripavlutin.com/react-hooks-stale-closures/

@YuriGor
Copy link
Author

YuriGor commented Apr 9, 2021

I forgot to mention I am working also with useEffect hook and adding event listeners manually, because I need active listeners (to be able to prevent default behavior). React creates all events listeners as passive and gives no way to make it active.

Here is a demo of the problem being discussed and possible solution I am currently using:
https://codesandbox.io/s/usestategetstate-c0g0x?file=/src/App.js

import React from "react";
import "./styles.css";
import { useRef, useEffect } from "react";
import useGetState from "./useGetState";
export default function App() {
  const ref1 = useRef(null);
  const ref2 = useRef(null);
  const [counter, setCounter, getCounter] = useGetState(0);

  useEffect(() => {
    if (ref1.current && !ref1.current.subscribed) {
      ref1.current.subscribed = true;
      ref1.current.addEventListener("pointerup", () => alert(counter), {
        passive: false
      });
    }
  }, [counter, ref1]);

  useEffect(() => {
    if (ref2.current && !ref2.current.subscribed) {
      ref2.current.subscribed = true;
      ref2.current.addEventListener("pointerup", () => alert(getCounter()), {
        passive: false
      });
    }
  }, [getCounter, ref2]);

  return (
    <div className="App">
      <button onClick={() => setCounter(counter + 1)}>
        Increment Counter: {counter}
      </button>
      <br />
      <button ref={ref1}>Alert counter state on not passive event</button>
      <button ref={ref2}>Alert counter getState() on not passive event</button>
    </div>
  );
}

Maybe I am doing something wrong and we still can use current state value directly as given by useState, but I found no better way.

I guess the fact, that there is such a form of setState, that is accepting callback and providing current state value is already proof of problem existence, otherwise why react supports this?

So what I suggest - just to expose already existing functionality of getting current state value directly, without wasting resources on wrapping it into setState method. I pretty often know for sure I will not change the state, so why should I make react doublecheck if new state in setState method arg is actually old one.
It would be good to just getState instead (please).

@cemreyavuz
Copy link

@YuriGor, one of the ways to achieve what you are trying to do is using the cleanup state of useEffect hook. When your counter state changes, you will remove the handler with the stale state and add a new one with the updated one. Something like that should work for your case:

useEffect(() => {
  const eventHandler = () => {
    alert(counter);
  }
  
  const node = ref1.current;
  if (node) {
    node.addEventListener("pointerup", eventHandler, {
      passive: false
    });
  }

  return () => {
    node.removeEventListener("pointerup", eventHandler, {
      passive: false
    });
  };
}, [counter]);

In addition, I'm not sure how complex your case is than the example in codesandbox but I want to note that you can also use the onPointerUp attribute of the button element. It would make your job much more easier. However, if you still need to add event listeners manually, you can use the given example above for sure.

@YuriGor
Copy link
Author

YuriGor commented Apr 9, 2021

Thank you @eusbolh, do you think adding / removing listeners is cheaper from performance perspective, comparing to just taking fresh state from setState?
Yes I need this events to be not passive, to preventDefault them, that's why I am adding them "manually".
There is a discussion about this - looks not very promising.

In my real app I am using mouse wheel and touch events to make zoom and pan, so I need to prevent default scrolling/zooming behavior, in order to apply my own.

I have "scale" "offsetX" and "offsetY" (and some others like minScale which is recalculating depending on window size) states, and to calculate matrix properly I need to consider all of them, but state variable itself is useless in my case and I have to take it from 'setState'.

Now you can imagine how many events is produced during simple "pinch" gesture.
I think it will be not very efficient to use such event listeners reattaching technique?
And second question - what is valid use cases for setState(callback) variant, if not one like mine?

Do you see any hidden drawbacks in the approach I am suggesting?
I am still learning react, but for now getState looks fine for me and works fast enough - it works fine in pretty complex combinations of hooks and native events.. If it would be implemented internally in useState hook - it would be even faster.

@cemreyavuz
Copy link

Do you see any hidden drawbacks in the approach I am suggesting?

I believe that what you are doing is a workaround rather than a solution. You might be getting away with your specific case, but it will get more complex as the codebase grows.

Let's assume you have a prop that needs to be used in that event handler. How would you manage to make that work? I created a sandbox for you to work on. For this example, you might think that you can come up with a similar workaround as you did with states (providing getters for props with the help of setState). How about the fetched data in redux (or any other state management library if you are using any)? You will not be able to use setState for those.

In theory, you can keep everything using the useState hook and make use of setState. However, at some point, you will need to remove that event listener and add a new one with the updated closure. This is caused by the way the hooks are designed. React relies on state and prop updates and trying to work with stale closures is not manageable in the long run in my opinion. What I mean is that the actual problem is not related to useState hook, it is trying to make stale closures work.

And second question - what is valid use cases for setState(callback) variant, if not one like mine?

The most common case for functional update syntax is the case that the new state is computed using the previous state. For example, if you are incrementing a counter, the new state is calculated as previous value of counter + 1 which is the equivalent of setState(prevCount => prevCount + 1). You can read this section of the React documentation.

do you think adding / removing listeners is cheaper from performance perspective, comparing to just taking fresh state from setState?

I don't have specific performance data related to these two approaches but I believe that adding and removing 4-5 event listeners on each render shouldn't cause any major performance problems. On each render, lots of other things happen besides the event listeners you add. It shouldn't matter that much if the number of event listeners doesn't get out of hand. If it does get out of hand, then that means you have bigger problems. In my opinion, you should try to optimize your code when you experience performance problems because working with stale closures will only get worse.

@YuriGor
Copy link
Author

YuriGor commented Apr 10, 2021

Good point about other props, thank you.
I am at risk of forwarding all the props I need into some state just to be able to access them,
but it looks very similar to (and worse then) already existing dependencies of useEffect hook.

I had an idea ,instead of adding custom native listeners directly, add one generic listener in some hook so this generic listener will route events to another layer of custom handlers declared inside render method via useEffect,
so on props/state changes listed in deps of useEffect, we will discard and recreate only these custom callbacks, not a generic listener, just to minimize interactions with native browser stuff and keep everything in scope of component.

It would be kind of getting action back to component, instead of floating away from it into separate independent entity with it's own life cycle.
I afaraid this way I am going to implement yet another react-like event liseners but with passive:false support, missing in original react.

@YuriGor
Copy link
Author

YuriGor commented Apr 10, 2021

Finally I just made a better use active listeners hook which is supporting deps but recreates only handlers, without reattaching listeners to dom node (I just feel safer by not doing this while, for example, touchmove is in progress)

import isEqual from 'lodash/isEqual';

const elements = new WeakMap();

export default function useActiveListeners({ current: element }, handlers, ...deps) {
  if (!element) return;
  if (!elements.has(element)) {
    const cfg = { deps };
    Object.entries(handlers).forEach(([name, handler]) => {
      const listener = function (e) {
        return listener.handler(e);
      };
      listener.handler = handler;
      cfg[name] = listener;
      element.addEventListener(name, listener, { passive: false });
    });
    elements.set(element, cfg);
  } else {
    const cfg = elements.get(element);
    if (!isEqual(cfg.deps, deps)) {
      Object.entries(handlers).forEach(([name, handler]) => {
        cfg[name].handler = handler;
      });
    }
  }
}

So a lot of redundant code was removed from my component and there is no more use cases for getState.
Thank you all for forcing me understand issue better instead of using workaround 👍

@YuriGor YuriGor closed this as completed Apr 10, 2021
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

3 participants