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

[feature] Add option to support late addition of headers #2123

Merged
merged 1 commit into from
Mar 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion doc/ws.md
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,8 @@ This class represents a WebSocket. It extends the `EventEmitter`.
- `address` {String|url.URL} The URL to which to connect.
- `protocols` {String|Array} The list of subprotocols.
- `options` {Object}
- `finishRequest` {Function} A function which can be used to customize the
headers of each http request before it is sent. See description below.
- `followRedirects` {Boolean} Whether or not to follow redirects. Defaults to
`false`.
- `generateMask` {Function} The function used to generate the masking key. It
Expand All @@ -316,12 +318,24 @@ This class represents a WebSocket. It extends the `EventEmitter`.
Options given do not have any effect if parsed from the URL given with the
`address` parameter.

Create a new WebSocket instance.

`perMessageDeflate` default value is `true`. When using an object, parameters
are the same of the server. The only difference is the direction of requests.
For example, `serverNoContextTakeover` can be used to ask the server to disable
context takeover.

Create a new WebSocket instance.
`finishRequest` is called with arguments

- `request` {http.ClientRequest}
- `websocket` {WebSocket}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this argument needed? The user already has access to the WebSocket instance returned by the constructor, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finishRequest is called from the constructor, so the new WebSocket hasn't been returned yet

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, but is it needed? What would it be used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no specific use-case, it just seemed polite to provide it to finishRequest just in case it's needed since it gets invoked before the caller has access to the websocket. It's not critical, but it takes no effort to pass it as argument while it would be annoying if it doesn't get passed and you do end up needing it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can add it later if someone needs it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, it might be useful if someone want to call websocket.close() or websocket.terminate() synchronously.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or they may just want to keep track of the WebSocket for which they're doing asynchronous work for bookkeeping/debugging purposes. Passing it just seemed like a sensible thing to do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, it might be useful if someone want to call websocket.close() or websocket.terminate() synchronously.

I think it doesn't make much sense 😄 . Why would anyone do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the end I don't care enough to continue debating it, I'll remove the websocket argument.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, keep it. I quoted my comment, not yours.


for each HTTP GET request (the initial one and any caused by redirects) when it
is ready to be sent, to allow for last minute customization of the headers. If
`finishRequest` is set then it has the responsibility to call `request.end()`
once it is done setting request headers. This is intended for niche use-cases
where some headers can't be provided in advance e.g. because they depend on the
underlying socket.

#### IPC connections

Expand Down
6 changes: 5 additions & 1 deletion lib/websocket.js
Original file line number Diff line number Diff line change
Expand Up @@ -989,7 +989,11 @@ function initAsClient(websocket, address, protocols, options) {
});
});

req.end();
if (opts.finishRequest) {
opts.finishRequest(req, websocket);
} else {
req.end();
}
}

/**
Expand Down
29 changes: 29 additions & 0 deletions test/websocket.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3857,6 +3857,35 @@ describe('WebSocket', () => {
agent
});
});

it('honors the `finishRequest` option', (done) => {
const wss = new WebSocket.Server({ port: 0 }, () => {
const ws = new WebSocket(`ws://localhost:${wss.address().port}`, {
finishRequest(request, websocket) {
process.nextTick(() => {
assert.strictEqual(request, ws._req);
assert.strictEqual(websocket, ws);
});
request.on('socket', (socket) => {
socket.on('connect', () => {
request.setHeader('Cookie', 'foo=bar');
request.end();
});
});
}
});

ws.on('close', (code) => {
assert.strictEqual(code, 1005);
wss.close(done);
});
});

wss.on('connection', (ws, req) => {
assert.strictEqual(req.headers.cookie, 'foo=bar');
ws.close();
});
});
});

describe('permessage-deflate', () => {
Expand Down