solution #1982
solution #1982
Conversation
There was a problem hiding this comment.
It looks like there's a critical issue with the event listener in your App component. The arrow function used in addEventListener creates a new function reference, which prevents the event listener from being correctly removed in componentWillUnmount. This can lead to memory leaks. Please define the event handler as a class method and use it consistently in both addEventListener and removeEventListener. Once you address this, your code will be in great shape! Keep up the good work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
src/App.tsx
Outdated
| componentWillUnmount(): void { | ||
| document.removeEventListener('keyup', (e: KeyboardEvent) => | ||
| this.handleKeypress(e), | ||
| ); |
There was a problem hiding this comment.
The event listener is not being removed correctly in componentWillUnmount. The arrow function used in addEventListener creates a new function reference, which is different from the one used in removeEventListener. To fix this, you should define the event handler as a class method and use it in both addEventListener and removeEventListener.
There was a problem hiding this comment.
Great job on your submission! 🎉 While your code is on the right track, there are a few critical issues that need attention to ensure robust functionality:
-
Event Listener Management: The
handleKeypressmethod currently returns a new function each time it is called. This leads to different function references being used inaddEventListenerandremoveEventListener, preventing the event listener from being correctly removed incomponentWillUnmount. To resolve this, definehandleKeypressas a class method without returning a function, and use it directly in bothaddEventListenerandremoveEventListener. -
Code Style: Your method naming and ternary operator formatting adhere to the guidelines, which is great! Keep up the good work on maintaining clean and readable code.
Despite these issues, the decision to approve your pull request is made to help you move forward with your learning. Please review the comments and address the critical issue with the event listener to improve your code. This will not only enhance the functionality but also prevent potential memory leaks in the future. Keep learning and refining your skills! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/App.tsx
Outdated
| handleKeypress() { | ||
| return (e: KeyboardEvent) => this.setState({ pressedKey: e.key }); |
There was a problem hiding this comment.
The handleKeypress method currently returns a new function each time it is called. This results in different function references being used in addEventListener and removeEventListener, which prevents the event listener from being correctly removed. Consider defining handleKeypress as a class method without returning a function, and use it directly in both addEventListener and removeEventListener.
src/App.tsx
Outdated
| } | ||
|
|
||
| componentDidMount(): void { | ||
| document.addEventListener('keyup', this.handleKeypress()); |
There was a problem hiding this comment.
Using this.handleKeypress() here calls the method and returns a new function, which is not the same reference used in removeEventListener. Define handleKeypress as a class method and use this.handleKeypress directly.
src/App.tsx
Outdated
| } | ||
|
|
||
| componentWillUnmount(): void { | ||
| document.removeEventListener('keyup', this.handleKeypress()); |
There was a problem hiding this comment.
Using this.handleKeypress() here calls the method and returns a new function, which is not the same reference used in addEventListener. Define handleKeypress as a class method and use this.handleKeypress directly.
DEMO LINK