-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add Windows named pipe support for client #1808
Conversation
|
||
if (address instanceof URL) { | ||
parsedUrl = address; | ||
websocket.url = address.href; | ||
} else if ((isWinPipe = /^ws\+unix:\/\/\\\\[.?]\\pipe\\/.test(address))) { |
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.
I'm not happy to skip the URL parser which is why there is no IPC support on Windows. This is not a valid URL.
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.
On second thought, I see your point, and such a URL does seem a little bit odd.
@@ -528,7 +551,12 @@ function initAsClient(websocket, address, protocols, options) { | |||
opts.auth = `${parsedUrl.username}:${parsedUrl.password}`; | |||
} | |||
|
|||
if (isUnixSocket) { | |||
if (isWinPipe) { | |||
const index = opts.path.indexOf(':', 11); |
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.
Wouldn't index
be incorrectly set to -1
if opts.path
is something like '\\.\pipe\x:/foo'
? '\\.\pipe\x'
is a valid named pipe.
@hyurl how about something like this: diff --git a/lib/websocket.js b/lib/websocket.js
index ce6f34c..7ab8677 100644
--- a/lib/websocket.js
+++ b/lib/websocket.js
@@ -475,9 +475,9 @@ function initAsClient(websocket, address, protocols, options) {
websocket.url = address;
}
- const isUnixSocket = parsedUrl.protocol === 'ws+unix:';
+ const isIPCUrl = parsedUrl.protocol === 'ws+unix:';
- if (!parsedUrl.host && (!isUnixSocket || !parsedUrl.pathname)) {
+ if (!parsedUrl.host && (!isIPCUrl || !parsedUrl.pathname)) {
throw new Error(`Invalid URL: ${websocket.url}`);
}
@@ -528,11 +528,18 @@ function initAsClient(websocket, address, protocols, options) {
opts.auth = `${parsedUrl.username}:${parsedUrl.password}`;
}
- if (isUnixSocket) {
- const parts = opts.path.split(':');
+ if (isIPCUrl) {
+ if (opts.path.startsWith('\\\\.\\pipe\\', 1)) {
+ const [socketPath, path] = opts.path.split(':/');
- opts.socketPath = parts[0];
- opts.path = parts[1];
+ opts.socketPath = socketPath.slice(1);
+ opts.path = path ? `/${path}` : '/';
+ } else {
+ const [socketPath, path] = opts.path.split(':');
+
+ opts.socketPath = socketPath;
+ opts.path = path;
+ }
}
let req = (websocket._req = get(opts));
Limitations:
|
@lpinca I was thinking about something like const ws = new WebSocket("ws+unix:some.sock"); // this syntax should work both on Windows and Unix
const ws = new WebSocket("ws+unix:C:\\some.sock"); // this syntax only works on Windows
const ws = new WebSocket("ws+unix://some.sock"); // this syntax only works on Unix All we have to do is adding |
The
WebSocket
client has been supporting Unix socket for a long time, but lacks the support of Windows named pipe,because in the previous versions, the URL constructor used to parse the address cannot parse a path in windows style,
and the following code will throw a TypeError:
To implement full support of IPC, I added two more steps for parsing the address and setting the socket path for connection,
now the above code can work as expected.