Feat: Add ability to set headers for WebSocket client

[marcus-j-davies]

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

Proposed changes

This PR adds the ability to set Headers for the WebSocket client.
a lot of the editor code was lifted from the HTTP Request node, with some small differences.

• Values are iether typed or can be taken from env, global or a static string
• Similarly, the header names can only be set from a typed value, or a static string

The hint text has been updated, to state that headers are only used during the Upgrade routine, and this message has been translated accordingly.

A few checks are in place, to ensure users who have not provided a headers object in the node (i.e they are in receipt of this change without making changes) does not cause an undefined error

Discussion here:
https://discourse.nodered.org/t/allow-adding-headers-to-websocket-client

Checklist

• I have read the contribution guidelines
• For non-bugfix PRs, I have discussed this change on the forum/slack team.
• I have run npm run test to verify the unit tests pass
• I have added suitable unit tests to cover the new/changed functionality

[marcus-j-davies]

@knolleary, @dceejay

Is there a reason why this PR isn't triggering other checks? where other PR's have?
This is my first PR on the project - maybe its that?

I can't think what I would have missed

[marcus-j-davies]

@Steve-Mcl, @knolleary

I have just noticed the dev branch is tagged for V4.

This PR checks for uninitialised properties of headers - as I assumed we will have hit another feature update before V4, therefore checking for uninitialised headers to stop it causing a break for implementations of the websocket client created in 3.1.x

node-red/packages/node_modules/@node-red/nodes/core/network/22-websocket.js

Line 103 in 7916dc9

if(node.upgradeHeaders !== undefined && node.upgradeHeaders.length > 0){

With the dev branch targeting V4, would you prefer I remove this check, and render it as a breaking change?

[Steve-Mcl]

Is there a reason why this PR isn't triggering other checks?

Not certain what you mean? As far as I can see, tests have ran?

With the dev branch targeting V4, would you prefer I remove this check, and render it as a breaking change?

We will evaluate the situation after 3.1.1 (when we prepare for v4)

---

Thanks for your efforts thus far.

[marcus-j-davies]

Not certain what you mean? As far as I can see, tests have ran?

it was addressed by @dceejay shortly after😄

[knolleary]

Thanks for this @marcus-j-davies - finally getting to reviewing this for 4.0

I'm not sure we need so many predefined headers in the list as many of them have no meaning in a WebSocket upgrade request. For example content-type will do nothing.

I suspect the main use-case is to be able to set the Authorization header - so maybe that should be the only predefined header provided. We should do some digging to see if there are any other commonly required headers.

As you note, you allow the value to be pulled from global and env. We would normally have both flow and global if we have any of them. In the backend, the call to evaluateNodeProperty then also must use an async callback as looking up a global value may require async work.

Given the time passed since you worked on this, I'm happy to do some of this rework as part of the 4.0 prep - but do let me know if you want to dust it off and make some of these updates yourself.

[marcus-j-davies]

Hi @knolleary

I'm not sure we need so many predefined headers in the list as many of them have no meaning in a WebSocket upgrade request. For example content-type will do nothing.

Agreed!
I'll reduce it down to the supported headers (I'll review the spec)

As you note, you allow the value to be pulled from global and env. We would normally have both flow and global if we have any of them. In the backend, the call to evaluateNodeProperty then also must use an async callback as looking up a global value may require async work.

The reason I have not allowed flow, msg or J; - is that this code block is part of the config node, so flow and msg are actually undefined here, this allowed me to escape the need for async, given JSONata is not used 😄

So IMO - please do correct me if wrong, is to reduce down the list of headers + any tweaks you feel are necessary?

Tweaks such as - should we really check for an undefined object for the headers array?

• If this Node should be a breaking change in v4
  • then no (ill remove the check)
• If it should not be a breaking change
  • then the check should remain... see : Feat: Add ability to set headers for WebSocket client #4436 (comment)

[knolleary]

There is absolutely no reason this should be a breaking change; it is adding a new feature, not changing an existing one. So the checks you have in place are entirely appropriate.

If we want to allow global context access, then it must use a callback in evaluateNodeProperty as global context could be backed by an async store.

It might be easier to just remove that option for now - nothing says we can't add more options later.

[marcus-j-davies]

Cool!

I'll push the edits a bit later.

[marcus-j-davies]

@knolleary

For now : I have commented out some of the header list items (I couldn't find any definitive, to determine what are valid headers for an upgrade operation).

I have also removed access to global content

[knolleary]

Looks good - have applied one small change to use the built-in env type, rather than redefining it. Will get this merged and into beta.0 this week!