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

resolves #85 introduce onBeforeDataWrite callback #86

Closed
wants to merge 1 commit into from
Closed

resolves #85 introduce onBeforeDataWrite callback #86

wants to merge 1 commit into from

Conversation

theogravity
Copy link
Contributor

@theogravity theogravity commented Aug 4, 2022

I'm introducing a callback called onBeforeDataWrite with a signature (data: Buffer) => Buffer that allows manipulation of the data before it is written out to the socket.

This resolves #85.

@@ -84,6 +86,10 @@ module.exports = function factory (userOptions) {
if (socket.writableEnded) {
handleSocketWriteError(new Error('This socket has been ended by the other party'), data, encoding)
} else {
if (options.onBeforeDataWrite) {
data = options.onBeforeDataWrite(data)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is bad practice here, but I've overwritten the existing data value to spare memory overhead from having to create another copy of the data.

If you're not into that, I can do something of the following:

let newData

if (options.onBeforeDataWrite) {
  newData = options.onBeforeDataWrite(data)
}

socket.write(newData | data, ...)

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina requested a review from jsumners August 4, 2022 07:40
lib/TcpConnection.js Outdated Show resolved Hide resolved
lib/TcpConnection.js Outdated Show resolved Hide resolved
Readme.md Outdated Show resolved Hide resolved
@theogravity
Copy link
Contributor Author

theogravity commented Aug 4, 2022

Addressed feedback.

  • Updated to typeof options.onBeforeDataWrite === 'function')
  • Added in the jsdoc and readme that the operations in the function must be sync.

@mcollina
Copy link
Member

mcollina commented Aug 5, 2022

why did you close this?

@theogravity
Copy link
Contributor Author

theogravity commented Aug 5, 2022

I didn't. Looks like it auto-closes if I remove my forked repo.

However, you should still be able to merge it?

If you're unable to, I can re-fork and re-create the PR by creating a patch from this one and applying it.

@theogravity
Copy link
Contributor Author

Odd, the Github app gives status saying it's mergable but the web UI completely removes it.

Apologies, will re-fork and re-submit.

This pull request was closed.
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.

Callback to manipulate the message before send?
3 participants