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

Replace socket.io with native WebSocket communication and update dependencies #24

Merged
merged 4 commits into from
Jan 1, 2024

Conversation

claygorman
Copy link
Contributor

@claygorman claygorman commented Jan 1, 2024

Type

enhancement, bug_fix


Description

  • Replaced socket.io implementation with native WebSocket communication in both frontend and backend.
  • Added new WebSocket hook useWsAuthenticatedSocket for frontend WebSocket communication.
  • Removed all instances of socket.io client and server-side code.
  • Implemented WebSocket server-side authentication and heartbeat mechanism.
  • Updated frontend and backend dependencies to reflect the removal of socket.io and addition of WebSocket related packages.

PR changes walkthrough

Relevant files                                                                                                                                 
Configuration changes
6 files
layout.tsx                                                                                                   
    frontend/app/layout.tsx

    Removed IoProvider import and usage, indicating removal of
    socket.io dependency.

+7/-8
DropdownNotification.tsx                                                                       
    frontend/components/Header/DropdownNotification.tsx

    Removed useAuthenticatedSocket and associated socket event
    handling.

+6/-21
index.tsx                                                                                                     
    frontend/components/Layout/index.tsx

    **- Removed useAuthenticatedSocket and associated socket
    event handling.

    • Added WebSocket connection status handling
      with useWsAuthenticatedSocket.**
+16/-15
index.tsx                                                                                                     
    frontend/components/Notifications/index.tsx

    Replaced useAuthenticatedSocket with
    useWsAuthenticatedSocket for WebSocket communication.

+6/-6
useAuthenticatedSocket.tsx                                                                   
    frontend/hooks/useAuthenticatedSocket.tsx

    Removed the entire useAuthenticatedSocket hook file,
    indicating a shift from socket.io to native WebSocket.

+0/-43
config.ts                                                                                                     
    frontend/services/config.ts

    Changed NEXT_PUBLIC_API_URL to be explicitly cast as a
    string.

+1/-1
Enhancement
8 files
index.tsx                                                                                                     
    frontend/components/Backlog/index.tsx

    **- Replaced useAuthenticatedSocket with
    useWsAuthenticatedSocket.

    • Changed PointerSensor to
      MouseSensor and TouchSensor.
    • Reorganized imports and
      components.
    • Removed updateIssue mutation usage.
    • Added
      sendJsonMessage call to send notifications over
      WebSocket.**
+19/-24
index.tsx                                                                                                     
    frontend/components/KanbanBoard/index.tsx

    **- Replaced useAuthenticatedSocket with
    useWsAuthenticatedSocket.

    • Reorganized imports and
      components.
    • Added sendJsonMessage call to send
      notifications over WebSocket.**
+21/-25
useWsAuthenticatedSocket.tsx                                                               
    frontend/hooks/useWsAuthenticatedSocket.tsx

    Added new useWsAuthenticatedSocket hook for WebSocket
    communication.

+41/-0
socket-handlers.ts                                                                                   
    frontend/services/socket-handlers.ts

    Updated socketMsgHandler to handle WebSocket messages and
    parse them if needed.

+3/-1
index.js                                                                                                       
    backend/src/index.js

    **- Removed fastify-socket.io and associated setup.

    • Added
      WebSocket server setup with authentication and heartbeat
      mechanism.
    • Removed old socket.io event handling.**
+118/-14
index.js                                                                                                       
    backend/src/resolvers/Board/index.js

    Replaced socket.io event emission with WebSocket broadcast
    in updateBoard mutation.

+8/-3
index.js                                                                                                       
    backend/src/resolvers/Issue/index.js

    Replaced socket.io event emission with WebSocket broadcast
    in updateIssue mutation.

+12/-5
ws-server.js                                                                                               
    backend/src/services/ws-server.js

    Added WebSocket server utility functions for handling
    connections and broadcasting messages.

+24/-0
Dependencies
4 files
package.json                                                                                               
    backend/package.json

    Removed fastify-socket.io and socket.io dependencies and
    added nanoid and typescript dependencies.

+4/-3
pnpm-lock.yaml                                                                                           
    backend/pnpm-lock.yaml

    Updated lock file with removed and added dependencies
    corresponding to WebSocket changes and new packages.

+32/-115
package.json                                                                                               
    frontend/package.json

    Removed socket.io-client and socket.io-react-hook
    dependencies and added react-use-websocket.

+1/-2
pnpm-lock.yaml                                                                                           
    frontend/pnpm-lock.yaml

    Updated lock file with removed and added dependencies
    corresponding to WebSocket changes and new packages.

+13/-105

Added react-use-websocket dependency and updated frontend service to handle websocket connections with authentication. Incorporated corresponding changes in the backend service to validate and respond to the authenticated requests. Also made necessary updates to frontend and backend files to support the new websocket service, and debugged new changes as well.
The code changes add a websocket functionality for when the board update mutation happens. This allows real-time updates across different users viewing the same board. Furthermore, the implementation includes a broadcast function that sends the updated board to all connected clients of the specific namespace. Additionally, a heartbeat is set up for maintaining and monitoring the connection status. A third-party library 'nanoid' has been added for generating unique client identifiers.
This commit refactors the websocket handling by removing the 'io' dependency in Board and Issue resolvers, replacing it with a more standardized 'websocketServer'. Board and Issue update methods no longer emit the old todo commented events, but instead use a 'websocketBroadcast' function to send update notifications. This function is defined in the 'ws-server.js' service. Additionally, some heartbeat and ping pong mechanisms were adjusted for better WebSocket connection stability. Also, necessary devDependencies were added in the 'package.json' file.
The code changes include removing files and code related to the socket.io library. The changes involve deleting related modules from package.json and pnpm-lock.yaml in both frontend and backend, removal of socket-related hooks and events, and removing all instances in different files where the socket was being used. The updates could be related to changes in the technical architecture or moving to a different library for websocket communication.
@claygorman
Copy link
Contributor Author

/describe

Copy link
Contributor

github-actions bot commented Jan 1, 2024

PR Analysis

  • 🎯 Main theme: Transition from socket.io to native WebSocket implementation
  • 📝 PR summary: This PR removes the dependency on socket.io and introduces a native WebSocket implementation using the 'react-use-websocket' library on the frontend and custom WebSocket handling on the backend. It includes refactoring of the existing code to accommodate the new WebSocket logic and removal of all socket.io related code.
  • 📌 Type of PR: Enhancement
  • ⏱️ Estimated effort to review [1-5]: 4, because the PR involves significant changes to the real-time communication infrastructure, which is a critical part of the application. It requires careful review of the new WebSocket implementation and ensuring that all instances of the previous socket.io implementation have been properly replaced and tested.
  • 🔒 Security concerns: Yes, because the PR introduces custom WebSocket handling which can be prone to security issues if not implemented correctly. The authentication process for WebSockets should be carefully reviewed to ensure that it is secure and does not expose sensitive information. Additionally, the lack of message validation could lead to security vulnerabilities such as injection attacks.

PR Feedback

💡 General suggestions: The transition from socket.io to native WebSocket implementation is a significant change that can improve the performance and reduce the complexity of the real-time communication features of the application. It is important to ensure that the new WebSocket implementation is robust, handles reconnections gracefully, and is secure. Additionally, thorough testing should be conducted to verify that all real-time features work as expected after the transition.

🤖 Code feedback:
relevant filefrontend/hooks/useWsAuthenticatedSocket.tsx
suggestion      

Consider adding error handling for the WebSocket connection to manage scenarios where the connection fails or closes unexpectedly. Implementing reconnection logic with exponential backoff could improve the resilience of the WebSocket communication. [important]

relevant lineshouldReconnect: (closeEvent) => true,

relevant filebackend/src/index.js
suggestion      

Ensure that the WebSocket authentication process is secure and robust. The current implementation retrieves the token from the 'sec-websocket-protocol' header, which is unconventional. Typically, a query parameter or a separate authentication message after establishing the connection is used for passing tokens. [important]

relevant lineconst token = request.headers['sec-websocket-protocol'].split(',').map((x) => x.trim())[1];

relevant filebackend/src/services/ws-server.js
suggestion      

Implement message validation in the 'handleConnection' function to ensure that incoming messages are in the expected format and contain valid data. This can prevent issues related to malformed messages and improve security. [important]

relevant line// socket.on('message', (message) => {

relevant filebackend/src/index.js
suggestion      

Consider using a library or framework for WebSocket communication that provides built-in features such as message validation, connection management, and scalability. This can reduce the amount of custom code needed and potentially improve the maintainability of the WebSocket implementation. [medium]

relevant linefastify.get('/ws', { websocket: true }, (connection /* SocketStream /, req / FastifyRequest */) => {

✨ Usage tips:

To invoke the PR-Agent, add a comment using one of the following commands:

  • /review: Request a review of your Pull Request.
  • /describe: Update the PR title and description based on the contents of the PR.
  • /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
  • /ask <QUESTION>: Ask a question about the PR.
  • /update_changelog: Update the changelog based on the PR's contents.
  • /add_docs 💎: Generate docstring for new components introduced in the PR.
  • /generate_labels 💎: Generate labels for the PR based on the PR's contents.
  • /analyze 💎: Automatically analyzes the PR, and presents changes walkthrough for each component.

See the tools guide for more details.
To edit any configuration parameter from the configuration.toml, add --config_path=new_value.
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, add a /config comment.

@github-actions github-actions bot changed the title Remove socket io Replace socket.io with native WebSocket communication and update dependencies Jan 1, 2024
@github-actions github-actions bot added enhancement New feature or request bug_fix labels Jan 1, 2024
Copy link
Contributor

github-actions bot commented Jan 1, 2024

PR Description updated to latest commit (55fb358)

@claygorman claygorman merged commit a3b74d5 into master Jan 1, 2024
2 checks passed
@claygorman claygorman deleted the remove-socket-io branch January 1, 2024 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant