othiym23 / async-listener

polyfill version of the 0.11 version of the asyncListener API
https://www.npmjs.org/package/async-listener
BSD 2-Clause "Simplified" License
174 stars 52 forks source link

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

Closed vdeturckheim closed 6 years ago

vdeturckheim commented 7 years ago

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 commented 7 years ago

👍

vdeturckheim commented 7 years ago

CI broken. I assume because of https://github.com/othiym23/async-listener/issues/109

joyeecheung commented 7 years ago

I think this is related to https://github.com/nodejs/node/commit/212a7a609d605c13294034f2f403632b626edf4c ?

EDIT: fixed by that commit?

vdeturckheim commented 7 years ago

Seems that it did not fully fix. Probably closing this when https://github.com/nodejs/node/pull/13069#pullrequestreview-38542738 gets merged

watson commented 7 years ago

@vdeturckheim Version 7.10.0 of Node.js doesn't have https://github.com/nodejs/node/commit/212a7a609d605c13294034f2f403632b626edf4c 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 commented 7 years ago

After looking over https://github.com/nodejs/node/commit/53f386932209ae375c3cf811329483e30868f3ac, 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 commented 7 years ago

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

watson commented 6 years ago

I think this is now fixed with #129 right?

vdeturckheim commented 6 years ago

lgtm @watson