I have this code:
const App: React.FC = () => {
const [isOpen, setIsOpen] = React.useState(true);
const [maxHeight, setMaxHeight] = React.useState();
const wrapper = React.useRef<HTMLDivElement>(null);
const content = React.useRef<HTMLDivElement>(null);
const setElementMaxHeight = () => {
if (content && content.current) {
setMaxHeight(isOpen ? content.current.offsetHeight : 0);
}
};
useEffect(() => {
setElementMaxHeight();
window.addEventListener("resize", setElementMaxHeight);
return () => {
window.removeEventListener("resize", setElementMaxHeight);
};
});
const toggle = () => {
setIsOpen(!isOpen);
};
return (
<div>
<button onClick={toggle}>
<span className="nominal-result__expander fa" />
</button>
<div
className="nominal-results__list-wrapper"
ref={wrapper}
style={!!maxHeight ? { maxHeight: `${maxHeight}px` } : undefined }
>
<div className="nominal-results__list" ref={content} />
</div>
</div>
);
};
const rootElement = document.getElementById("root");
ReactDOM.render(<App />, rootElement);
This will add and remove an event handler on each render.
Is this necessarily bad and does this actually gain anything from being a hook?
This came up in a code review and I am saying it is bad because it adds and removes the event listener on every render.
For this exact case you're right because undefined is passed as the dependencies of useEffect.
This means useEffect runs on every render and thus the event handlers will unnecessarily get detached and reattached on each render.
function listener() {
console.log('click');
}
function Example() {
const [count, setCount] = window.React.useState(0);
window.React.useEffect(() => {
console.log(`adding listener ${count}`);
window.addEventListener("click", listener);
return () => {
console.log(`removing listener ${count}`);
window.removeEventListener("click", listener);
};
}); // <-- because we're not passing anything here, we have an effect on each render
window.React.useEffect(() => {
setTimeout(() => {
setCount(count + 1);
}, 1000)
});
return count;
}
window.ReactDOM.render(window.React.createElement(Example), document.getElementById('root'))
<script crossorigin src="https://unpkg.com/react@16/umd/react.development.js"></script>
<script crossorigin src="https://unpkg.com/react-dom@16/umd/react-dom.development.js"></script>
<div id="root"></div>
But if you explicitly declare no dependencies by passing in an empty array [], useEffect will only run once, thus making this pattern perfectly legitimate for event handler attachment.
function listener() {
console.log('click');
}
function Example() {
const [count, setCount] = window.React.useState(0);
window.React.useEffect(() => {
console.log(`adding listener ${count}`);
window.addEventListener("click", listener);
return () => {
console.log(`removing listener ${count}`);
window.removeEventListener("click", listener);
};
}, []); // <-- we can control for this effect to run only once during the lifetime of this component
window.React.useEffect(() => {
setTimeout(() => {
setCount(count + 1);
}, 1000)
});
return count;
}
window.ReactDOM.render(window.React.createElement(Example), document.getElementById('root'))
<script crossorigin src="https://unpkg.com/react@16/umd/react.development.js"></script>
<script crossorigin src="https://unpkg.com/react-dom@16/umd/react-dom.development.js"></script>
<div id="root"></div>
