DEV Community

loading...

Why pressing the key works only once in my React project?

ptifur profile image Yar ・1 min read

Hey there!

Could you tell me what's wrong with this code?

I have a button in my application that toggles the state.

I would like to achieve the same with pressing the spaсebar on the keyboard. And it works only one way. The state changes to false once. And then no reaction.

import { useState, useEffect } from 'react'

const HandleKeypress = () => {

    const [itWorks, setItWorks] = useState(true)

    useEffect(() => {
        document.addEventListener('keypress', (e) => {
            if (e.code === 'Space') setItWorks(!itWorks)
        })
    }, [])

    return (
        <div>
            <p>{itWorks ? 'It works!' : 'It does not'}</p>
            <button 
                onClick={() => setItWorks(!itWorks)}
            >Press me</button>
        </div>
    )
}

export default HandleKeypress
Enter fullscreen mode Exit fullscreen mode

What am I missing? 😼

Discussion (12)

pic
Editor guide
Collapse
merri profile image
Vesa Piittinen • Edited

The code within useEffect(() => {}, []) is executed only once. The value for itWorks upon creation of the function is true and consequently it will always be true.

There are two ways you can fix this.

Method 1: update listener each time it changes.

import { useState, useEffect } from 'react'

const HandleKeypress = () => {

    const [itWorks, setItWorks] = useState(true)

    useEffect(() => {
        // reference must be same for addEventListener and removeEventListener
        // = can't use inline arrow function!
        function listener(event) {
            if (event.code === 'Space') setItWorks(!itWorks)
        }
        document.addEventListener('keypress', listener)
        return () => {
            document.removeEventListener('keypress', listener)
        }
    }, [itWorks])

    return (
        <div>
            <p>{itWorks ? 'It works!' : 'It does not'}</p>
            <button 
                onClick={() => setItWorks(!itWorks)}
            >Press me</button>
        </div>
    )
}

export default HandleKeypress
Enter fullscreen mode Exit fullscreen mode

This works because now each time that itWorks changes the listener is updated.

Method 2: use setState with a function

import { useState, useEffect } from 'react'

const HandleKeypress = () => {

    const [itWorks, setItWorks] = useState(true)

    useEffect(() => {
        document.addEventListener('keypress', (e) => {
            if (e.code === 'Space') setItWorks(state => !state)
        })
    }, [])

    return (
        <div>
            <p>{itWorks ? 'It works!' : 'It does not'}</p>
            <button 
                onClick={() => setItWorks(!itWorks)}
            >Press me</button>
        </div>
    )
}

export default HandleKeypress
Enter fullscreen mode Exit fullscreen mode

The function gets the current value in the state so if you're only toggling it you can simply reverse it. This works for all cases where you determine the next state based on the previous state.

Collapse
ptifur profile image
Yar Author

Thanks for the insights! With these corrections both methods work indeed!

Is there a reason to prefer one over the other?

Collapse
merri profile image
Vesa Piittinen

The second one causes slightly less work for CPU to do, although the effect is quite minimal with today's computing power. And the work only happens when the value changes, in this case from user action, so not really a big argument for it.

However it is always a good idea to clean up what you create to the DOM, so in this case by adding the removeEventListener part even if using the second approach.

Thread Thread
ptifur profile image
Yar Author

Thank you! Makes sense.

Collapse
snorkypie profile image
Steeve Lennmark

You still want to remove the event listener on unmount in the second example though!

Collapse
tomburgs profile image
Toms Burgmanis

You're suffering from stale props.

A solution would be to pass your prop as an argument to useEffect, however, as you're adding an event listener this would cause you to also remove it every time the argument updates. Constantly doing that will hurt performance.

What instead you want is to pass the setItWorks function a callback instead that will be passed the latest state when it's called.

However, you should also always remove event listeners when your component unmounts.

The following code will fix your issue:

const [itWorks, setItWorks] = useState(true);

useEffect(() => {
  const handleItWorks = ({ code }) => {
    if (code === 'Space') {
      setItWorks(newItWorks => !newItWorks);
    }
  };

  document.addEventListener('keypress', handleItWorks);

  return () => {
    document.removeEventListener('keypress', handleItWorks);
  };
}, [setItWorks]);
Enter fullscreen mode Exit fullscreen mode
Collapse
ptifur profile image
Yar Author

Thanks for the explanation! I appreciate it.

Collapse
joelnet profile image
JavaScript Joel

This looks like an issue with stale values inside of useEffect.

Make this change:

// ❌ Stale value
if (e.code === 'Space') setItWorks(!itWorks)

// βœ… Correct value
if (e.code === 'Space') setItWorks(state => !state)
Enter fullscreen mode Exit fullscreen mode

I have created a video on this topic here:

Cheers 🍻

Collapse
ptifur profile image
Yar Author

Thanks Joel! :)

Collapse
erandakarachchi profile image
Eranda Kudalugodaarachchi
useEffect(() => {
    document.addEventListener('keypress', (e) => {
        if (e.code === 'Space') setItWorks(!itWorks)
    })
}, [itWorks])
Enter fullscreen mode Exit fullscreen mode

This might work. But haven't tested though.

Collapse
ptifur profile image
Yar Author

Thanks Eranda! Seems like it doubles the number of added key listeners with each press and soon the application stops responding.

Which means the answer is to remove keyListeners and clean up useEffect an some point? Not sure how, I've reached the limit of my Hooks understanding 😜

Collapse
erandakarachchi profile image
Eranda Kudalugodaarachchi • Edited

When we add itWorks as a dependency to useEffect, It will re-render for each change of itWorks value. So here it will cause the number of event listeners to multiply at each change.
So as @Vesa Piittinen suggests it is a good idea to remove the listener if you are adding it in the useEffect hook.