-
Notifications
You must be signed in to change notification settings - Fork 503
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: async connect #887
feat: async connect #887
Conversation
Fixes: #885
Codecov Report
@@ Coverage Diff @@
## main #887 +/- ##
=======================================
Coverage 99.81% 99.81%
=======================================
Files 26 26
Lines 2121 2132 +11
=======================================
+ Hits 2117 2128 +11
Misses 4 4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code looks good, could you also take a look at benchmarks?
@mcollina Seems like there's no performance regression. There is always some difference in various tests in other runs but they're heading for about the same value. Node.js v16.5.0 async connect:
main:
|
This wrapper should provide support for native Node.js agents: https://gist.github.com/szmarczak/a4b23a3cd6f402b2de41e6eed0bcd011 Usage of
The bad thing here is that some of the agents above try to mutate the |
6679898
to
4b5ed0b
Compare
* feat: async connect Fixes: nodejs#885 * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixuP
Fixes: #885