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

Allow to pass socket options to TCP client #519

Merged
merged 7 commits into from
Oct 19, 2023
Merged

Conversation

AndreMaz
Copy link
Contributor

@AndreMaz AndreMaz commented Oct 18, 2023

Hi @yaacov thank you for the lib. It's been super useful

While doing some debugging I've made some changes to the lib and decided to create a PR.

Here are the changes:

  1. allows to pass socket opts (link to docs: https://nodejs.org/api/net.html#new-netsocketoptions) to the TCP client. When creating new Socket()
  2. allows to pass TCP connections (link to docs: https://nodejs.org/api/net.html#socketconnectoptions-connectlistener) when calling _client.connect()
  3. adds a test coverage reporter that helps to see uncovered codebase
    image
  4. adds a vscode launch.json that allows to attach a debugger to unit tests and to the examples in examples dir
  5. adds and example showing how to use 1) to pass abort signal to the Socket() and easily close the socket.

Re 5) I have a question for you. When the socket emits an error and socket connection is closed, does it make sense to wait for a response (which won't come as socket is closed) and then throw a TransactionTimedOutError ?
image

.gitignore Outdated Show resolved Hide resolved
@AndreMaz
Copy link
Contributor Author

It fails for node v14 because AbortController was introduced in node v15 (source: https://developer.mozilla.org/en-US/docs/Web/API/AbortController#browser_compatibility) . However, node 14 reached EOL at April 30, 2023

@yaacov
Copy link
Owner

yaacov commented Oct 19, 2023

It fails for node v14 because AbortController was introduced in node v15 (source: https://developer.mozilla.org/en-US/docs/Web/API/AbortController#browser_compatibility) . However, node 14 reached EOL at April 30, 2023

I am ok with removing v14 from the test matrix

@yaacov yaacov closed this Oct 19, 2023
@yaacov yaacov reopened this Oct 19, 2023
@yaacov
Copy link
Owner

yaacov commented Oct 19, 2023

Thank you for this pull request 💚

@AndreMaz
Copy link
Contributor Author

@yaacov dropped node v14. Thank you for reviewing the code.

@yaacov yaacov merged commit 9a74d61 into yaacov:master Oct 19, 2023
2 checks passed
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