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

update argument management in net.Socket.prototype.connect to handle node 8 #110

Closed
wants to merge 1 commit into from

Conversation

vdeturckheim
Copy link
Contributor

Tested with node v8.0.0-rc.0.

The signature of net._normalizeArgs has been changed in node core. I belive it leads to an issue in async-listner:

I have been having issue while performing http request from a process where the CLS in instanciated (https://gist.github.com/vdeturckheim/5d96705298a55a90a25716314edd449e).

This change fixes my issue and fixes the tests of async-listener with node v8.0.0-rc.0.

CC @joyeecheung @mcollina @jasnell

@mcollina
Copy link

👍

@vdeturckheim
Copy link
Contributor Author

CI broken. I assume because of #109

@joyeecheung
Copy link

joyeecheung commented May 16, 2017

I think this is related to nodejs/node@212a7a6 ?

EDIT: fixed by that commit?

@vdeturckheim
Copy link
Contributor Author

Seems that it did not fully fix.
Probably closing this when nodejs/node#13069 (review) gets merged

@watson
Copy link
Collaborator

watson commented May 17, 2017

@vdeturckheim Version 7.10.0 of Node.js doesn't have nodejs/node@212a7a6 in it, so I'm pretty sure that's why CI is failing.

@MylesBorins do you know when the next 7.x release is cut?

@watson
Copy link
Collaborator

watson commented May 17, 2017

After looking over nodejs/node@53f3869, I can't see how net._normalizeArgs can ever have an array as the first element in the returned array. Even with the linked commit it seems to always be options. Are you sure this isn't a side-effect of something else? Or maybe I'm just overlooking something really simple here 😅

@vdeturckheim
Copy link
Contributor Author

I got it through experiments ^^ It seems that the PR on node will fix that better than mine anyway ;)

@watson
Copy link
Collaborator

watson commented Dec 21, 2017

I think this is now fixed with #129 right?

@watson watson closed this Dec 21, 2017
@vdeturckheim
Copy link
Contributor Author

lgtm @watson

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.

4 participants