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

feat: add options for controlling buffer usage #1414

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ejose19
Copy link

@ejose19 ejose19 commented Nov 27, 2020

Note: the socket.io.js file is the generated output of make socket.io.js, and should not be manually modified.

The kind of change this PR does introduce

  • a bug fix
  • a new feature
  • an update to the documentation
  • a code change that improves performance
  • other

Current behaviour

Currently, users cannot opt-out of buffer usage, which may not be wanted if they need the events to not be delayed until a reconnection is made (for receive events). For send events it would act like volatile but for all events.

New behaviour

Users can control buffer usage

Other information (e.g. related issues)

@darrachequesne
Copy link
Member

With these options, wouldn't it be necessary for the user to always check the value of socket.connected (as the packet might be silently discarded)?

In other words, what is the difference between:

// enableSendBuffer: true
if (socket.connected) { // prevents the event from being added to the buffer
  socket.emit(/* ... */);
}

and

// enableSendBuffer: false
if (socket.connected) { // prevents the event from being silently discarded
  socket.emit(/* ... */);
}

@ejose19
Copy link
Author

ejose19 commented Jan 15, 2021

Well, from what I saw the buffer is only used (as adding packets) when the socket is not connected, so were it would be an issue?

If the user explicitly set enableSendBuffer: false then he's aware that if no connection is established the packet will be discarded, the idea of enableSendBuffer: false is that all packets works like they're volatile (instead of setting volatile in each event)

@ejose19
Copy link
Author

ejose19 commented Oct 28, 2021

Anything you need for merging this PR @darrachequesne? I'd like to drop the .patch and have this integrated natively.

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.

2 participants