Skip to content

fix: multiple connections issue#3

Open
focux wants to merge 3 commits into
iamgyz:masterfrom
focux:fix-multiple-connections
Open

fix: multiple connections issue#3
focux wants to merge 3 commits into
iamgyz:masterfrom
focux:fix-multiple-connections

Conversation

@focux

@focux focux commented Mar 23, 2020

Copy link
Copy Markdown

This pull request aims to fix #1. I saw an old PR (#2) trying to fix this same issue using refs, but the problem with that was that it was setting the ref inside the render function and not inside an useEffect hook, therefore, it was creating a new connection of socket.io in each render.

The approach that I followed was creating and storing the instance of socket.io inside an useEffect hook. The only caveat that this approach has is that in the first render it will return a null value and after that, the socket instance; this because of the useEffect hook.

@zilahir

zilahir commented Mar 23, 2020

Copy link
Copy Markdown

@focux this is clever! i have tested your fix, and by wrapping the socket in if (socket), works like charm! :)

thank you for your contribution!

@realdennis realdennis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, overall is good, the last pr #2 did not concern about ref will change in render function, I thought your approach is good to fix the #1 , since the return array is empty so the instance will be created once.

But please check L8, I think the return value would be function instead socket instance.

Comment thread src/index.js Outdated
Comment thread src/index.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

This connects on every render

3 participants