fix: wrap net.connect failed in node.js 8+ #129
Conversation
|
ci failed, I looked |
|
related PR: #110 |
|
@sabakugaara Thanks for taking the time to look into this 😃 Unless someone else have time, I'll try and take a look within the next few days when I'm back home |
|
Hi @watson, I update the test case, CI has been passed now. |
index.js
Outdated
| @@ -105,10 +106,16 @@ function patchOnRead(ctx) { | |||
|
|
|||
| wrap(net.Socket.prototype, 'connect', function (original) { | |||
| return function () { | |||
| var args; | |||
| if (v8plus && Array.isArray(arguments[0])) { | |||
There was a problem hiding this comment.
Here's the related code in Node 8:
https://github.com/nodejs/node/blob/0d8021e5a4cf0a6aa3a700a361f6d42c2894f2ba/lib/net.js#L943-L951
As you can see Node core uses an internal symbol referenced by normalizedArgsSymbol (which we don't have access to) as an extra guard in case someone actually by a error passes in an array as the first argument. First I thought we couldn't check for at as the normalizedArgsSymbol symbol isn't available to us, but I just realized that we could add a check for if there's a symbol or not - which might be better than nothing:
if (v8plus && Array.isArray(arguments[0]) && Object.getOwnPropertySymbols(arguments[0]).length > 0) {We could also go all the way to to a string comparison, but I feel that's way too dirty:
Object.getOwnPropertySymbols(arguments[0]).toString() === 'Symbol(normalizedArgs)'I could also be fine with just merging the PR as is. But I just wanted to follow this train of thoughts 😅
@Qard what do you think?
There was a problem hiding this comment.
I think it might have Symbol.iterator on it, so just checking for presence of any symbols probably won't be helpful. We can probably just drop that check and just go by if it's an array.
There was a problem hiding this comment.
so, just keep it the same as this is acceptable?
if(v8plus && Array.isArray(arguments[0])) {
There was a problem hiding this comment.
A normal array will not have a Symbol, so this should still guard against that case. So I think I'll merge this PR in as is and then add a comment about the Symbol gotcha next to the if-sentence. Thanks for the patience 😄
|
Thanks @watson , I've updated the code. |
|
Do I need to update anything more? |
in node8.0+,
Socket.prototype.connectarguments has been normalized innet.connectcall.This will fix node-continuation-local-storage test broken.
possible related issue: othiym23/node-continuation-local-storage#117