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

Callbacks do not care about dependency list #687

Open
ryancrunchi opened this issue Jul 4, 2022 · 5 comments
Open

Callbacks do not care about dependency list #687

ryancrunchi opened this issue Jul 4, 2022 · 5 comments

Comments

@ryancrunchi
Copy link

ryancrunchi commented Jul 4, 2022

When I use CountUp with an onEnd callback with dependency list of variables, the values of theses variables are always the same initial ones. Because useCountUp hook does not react to props changes as defined in

const start = useEventCallback(() => {
const run = () =>
getCountUp(true).start(() => {
onEnd?.({ pauseResume, reset, start: restart, update });
});
if (delay && delay > 0) {
timerRef.current = setTimeout(run, delay * 1000);
} else {
run();
}
onStart?.({ pauseResume, reset, update });
});

Maybe the useEventCallback hook is not the good place to be used here as the callbacks of countup can be redefined during rendering. useEventCallback is used when the function is ensured to not be called during render. Which is not the case with countup. We can redefined onEnd while the countup animation is in progress.

In the following example you can see in console that the value or initialValue are always 0. Despite they should go up at each countup end event.

Expected behaviour : initialValue takes the value of value at the end of animation
Current behaviour : initialValue is always 0 because value in handleEnd callback is not updated

https://codesandbox.io/s/react-typescript-forked-7leiul?file=/src/MyComponent.tsx

PS: I know countup has an update function and should not be used the way this example is. But it doesn’t change the fact that callbacks always have outdated values in dependency list. Also, in my production app, I have a more complex use case with cached data as props, that update a counter state and this counter update another state on end which also depends on cached data (set in dependency list). But with current implementation my cached data are the old one in the callback due to this issue above. And the state is not updated accordingly at end of countup animation.

@mmarkelov
Copy link
Collaborator

@ryancrunchi I'm not 100% sure, but I guess that there is preserveValue which can help with your case.

import { useCallback, useState } from "react";
import CountUp from "react-countup";
import "./styles.css";

export default function App() {
  const initialValue = 0; 
  const [value, setValue] = useState(initialValue);

  const handleClick = useCallback(() => {
    setValue((old) => old + Math.round(Math.random() * 100));
  }, []);

  return (
    <div className="App">
      <CountUp preserveValue start={initialValue} delay={0} end={value}>
        {({ countUpRef }) => <span ref={countUpRef} />}
      </CountUp>{" "}
      <button onClick={handleClick}>Test</button>
    </div>
  );
}

@ryancrunchi
Copy link
Author

Ok but that’s not going to solve the case described in the issue. The onEnd callback always have old state/props. It’s not a matter of countup values, but the way it uses callbacks
I will try to implement a more complex case to show you that

@ryancrunchi
Copy link
Author

ryancrunchi commented Jul 5, 2022

The example code sandbox has been updated with a more complex example
https://codesandbox.io/s/react-typescript-forked-7leiul?file=/src/MyComponent.tsx

@mmarkelov
Copy link
Collaborator

@ryancrunchi ok, I got your point. I guess that you are right about this. Will try to look through the fix soon. For now you can try this hack:

import { useCallback, useEffect, useMemo, useRef, useState } from "react";
import CountUp from "react-countup";

export type ComponentProps = {
  items: { quantity: number }[];
};

const MyComponent = ({ items, ...rest }: ComponentProps) => {
  // copy the items to own state to update them as needed
  const [myItems, setMyItems] = useState(items);
  const [disabled, setDisabled] = useState(false);

  const total = useMemo(() => myItems.reduce((acc, v) => acc + v.quantity, 0), [
    myItems
  ]);
  const [initialValue] = useState(total);

  useEffect(() => {
    if (total === 0) {
      setTimeout(() => {
        setDisabled(true);
        // 2000 - default duration
      }, 2000);
    }
  }, [total]);

  const handleClick = useCallback(() => {
    setMyItems((old) =>
      old.map((i) => ({ quantity: Math.max(0, i.quantity - 100) }))
    );
  }, []);

  return (
    <div>
      <CountUp preserveValue delay={0} start={initialValue} end={total}>
        {({ countUpRef }) => (
          <span
            style={{ color: disabled ? "red" : undefined }}
            ref={countUpRef}
          />
        )}
      </CountUp>
      <button disabled={disabled} onClick={handleClick}>
        Test
      </button>
    </div>
  );
};

export default MyComponent;

@ryancrunchi
Copy link
Author

ryancrunchi commented Jul 6, 2022

I found another workaround, maybe cleaner without timeout

const [refreshValue, refresh] = useReducer(x => x+1, 0)

const handleEnd = useCallback(() => {
    console.log("end", { total });
    refresh()
}, [total]);

useEffect(() => {
  setDisabled(total === 0)
  // don't set total in dependency or it will update as soon as total changes, what we don't want
}, [refreshValue])

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

2 participants