I am having a hard time understanding why the onClick event handler (which invokes 2 calls to a custom hook wrapper function) is not responding properly. I expect that everytime I click the button in the example would swap its border color from green to red based on a value that is being incremented. I understand the example is rudimentary and could easily be solved by conditioning the error prop on the value.value instead of sharing , but this is a simplified example of a more complex interaction, and I have boiled down the issue to a simple example for clarification. Any help would be appreciated. https://codesandbox.io/s/custom-hooks-with-closure-issue-2fc6g?file=/index.js
index.js
import useValueErrorPair from "./useValueErrorPair";
import styled from "styled-components";
import ReactDOM from "react-dom";
import React from "react";
const Button = styled.button`
background-color: black;
padding: 10px;
color: white;
${props =>
props.error ? "border: 3px solid #ff0000;" : "border: 3px solid #00ff00;"}
`;
const e = React.createElement;
const DemoComponent = () => {
const [value, setValue, setError] = useValueErrorPair(0, false);
console.log(value);
return (
<Button
error={value.error}
onClick={e => {
e.preventDefault();
setError((value.value 1) % 2 === 1); // If number of clicks is odd => error.
setValue(value.value 1); // Increment the state hook for value.
}}
>
Click Me For Problems!
</Button>
);
};
const domContainer = document.querySelector("#root");
ReactDOM.render(e(DemoComponent), domContainer);
export default DemoComponent;
useValueErrorPair.js
import { useState } from "react";
const useValueErrorPair = (initialValue, initialError) => {
const [v, setV] = useState({ value: initialValue, error: initialError });
const setValue = newValue => {
setV({ error: v.error, value: newValue });
};
const setError = newError => {
if (newError !== v.error) setV({ error: newError, value: v.value });
};
return [v, setValue, setError];
};
export default useValueErrorPair;
Snippet:
const { useState } = React;
const useValueErrorPair = (initialValue, initialError) => {
const [v, setV] = useState({ value: initialValue, error: initialError });
const setValue = newValue => {
setV({ error: v.error, value: newValue });
};
const setError = newError => {
if (newError !== v.error) setV({ error: newError, value: v.value });
};
return [v, setValue, setError];
};
const DemoComponent = () => {
const [value, setValue, setError] = useValueErrorPair(0, false);
console.log(value);
return (
<button type="button" className={value.error ? "error" : "okay"}
onClick={e => {
e.preventDefault();
setError((value.value 1) % 2 === 1); // If number of clicks is odd => error.
setValue(value.value 1); // Increment the state hook for value.
}}
>
Click Me For Problems!
</button>
);
};
const domContainer = document.querySelector("#root");
const e = React.createElement;
ReactDOM.render(e(DemoComponent), domContainer);.error {
border: 1px solid red;
}
.okay {
border: 1px solid green;
}<div id="root"></div>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react/17.0.2/umd/react.development.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react-dom/17.0.2/umd/react-dom.development.js"></script>CodePudding user response：
The problem is that your setter functions are using stale state. When setting new state based on existing state, you should use the callback form so you're always dealing with up-to-date information. In your case, the call to setError was working fine, but then the call to setValue was using a stale copy of v and undoing the change that setError had made.
If we use the callback form, the problem disappears, see *** comments:
const useValueErrorPair = (initialValue, initialError) => {
const [v, setV] = useState({ value: initialValue, error: initialError });
const setValue = newValue => {
// *** Use the callback form when setting state based on existing state
setV(({error}) => ({error, value: newValue}));
};
const setError = newError => {
// *** Again
setV(prev => {
if (newError !== prev.error) {
return { error: newError, value: prev.value };
}
// No change
return prev;
});
};
return [v, setValue, setError];
};
const { useState } = React;
const useValueErrorPair = (initialValue, initialError) => {
const [v, setV] = useState({ value: initialValue, error: initialError });
const setValue = newValue => {
// *** Use the callback form when setting state based on existing state
setV(({error}) => ({error, value: newValue}));
};
const setError = newError => {
// *** Again
setV(prev => {
if (newError !== prev.error) {
return { error: newError, value: prev.value };
}
// No change
return prev;
});
};
return [v, setValue, setError];
};
const DemoComponent = () => {
const [value, setValue, setError] = useValueErrorPair(0, false);
console.log(value);
return (
<button type="button" className={value.error ? "error" : "okay"}
onClick={e => {
e.preventDefault();
setError((value.value 1) % 2 === 1); // If number of clicks is odd => error.
setValue(value.value 1); // Increment the state hook for value.
}}
>
Click Me, It's Working!
</button>
);
};
const domContainer = document.querySelector("#root");
const e = React.createElement;
ReactDOM.render(e(DemoComponent), domContainer);.error {
border: 1px solid red;
}
.okay {
border: 1px solid green;
}<div id="root"></div>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react/17.0.2/umd/react.development.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react-dom/17.0.2/umd/react-dom.development.js"></script>There's another advantage to doing that: You can make the setter functions stable, like the ones you get from useState, rather than recreating them every time (which can have knock-on effects causing components to re-render unnecessarily). For hooks, I prefer to use refs for stability rather than useMemo (or useCallback, which uses useMemo) since the useMemo docs say it's not a semantic guarantee. (It also reduces the number of functions you create and throw away.)
Here's what that would look like:
const useValueErrorPair = (initialValue, initialError) => {
const [v, setV] = useState({ value: initialValue, error: initialError });
const settersRef = useRef(null);
if (!settersRef.current) {
settersRef.current = {
setValue: newValue => {
setV(({error}) => ({error, value: newValue}));
},
setError: newError => {
setV(prev => {
if (newError !== prev.error) {
// Update
return { error: newError, value: prev.value };
}
// No change
return prev;
});
},
};
}
return [v, settersRef.current.setValue, settersRef.current.setError];
};
Live Example:
const { useState, useRef } = React;
const useValueErrorPair = (initialValue, initialError) => {
const [v, setV] = useState({ value: initialValue, error: initialError });
const settersRef = useRef(null);
if (!settersRef.current) {
settersRef.current = {
setValue: newValue => {
setV(({error}) => ({error, value: newValue}));
},
setError: newError => {
setV(prev => {
if (newError !== prev.error) {
// Update
return { error: newError, value: prev.value };
}
// No change
return prev;
});
},
};
}
return [v, settersRef.current.setValue, settersRef.current.setError];
};
const DemoComponent = () => {
const [value, setValue, setError] = useValueErrorPair(0, false);
console.log(value);
return (
<button type="button" className={value.error ? "error" : "okay"}
onClick={e => {
e.preventDefault();
setError((value.value 1) % 2 === 1); // If number of clicks is odd => error.
setValue(value.value 1); // Increment the state hook for value.
}}
>
Click Me, It's Working!
</button>
);
};
const domContainer = document.querySelector("#root");
const e = React.createElement;
ReactDOM.render(e(DemoComponent), domContainer);.error {
border: 1px solid red;
}
.okay {
border: 1px solid green;
}<div id="root"></div>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react/17.0.2/umd/react.development.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react-dom/17.0.2/umd/react-dom.development.js"></script>