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

refactor(request): abstract https server into a tls server for protocol checks #311

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

J4Numbers
Copy link

The TLS.Server instance is common for both the https library and the http2 library,
according to the NodeJS documentation. Since it is a common ancestor, we should check for
this instance instead of the specific https flavour.

On a similar note, the net.Server instance is common for both http and http2 too,
though no changes are required to ensure they're cast to the same.

See the following links:

…ol checks

The TLS.Server instance is common for both the https library
and the http2 library, according to the NodeJS documentation.
Since it is a common ancestor, we should check for this instance
instead of the specific https flavour

Signed-off-by: Jayne Doe <jayne.doe@engineering.digital.dwp.gov.uk>
@J4Numbers
Copy link
Author

Looks to resolve #245 through allowing the http2 server instance as the Superagent instance behind chai-http is now updated to a supported version.

@keithamus
Copy link
Member

I've been out of the loop a while on node server stuff. Are there ramifications to this? Compatibility issues? What version of Node supports this? How will it effect our existing userbase? Should this be semver major?

@J4Numbers
Copy link
Author

I was testing this on Node 20.0.9. From the links in the main description, http2 has been around since 8.4.0 of NodeJS, so we're probably safe on that front for compatability (tls.server itself has been around since 0.3.4).

Functionaly, it's a non-breaking support change that could be argued into a patch to ensure that all https-compliant servers are called using https protocols. I can write supporting tests for both https and http2 if required, though that will require some meddling with certs for the repo (and I noticed that https wasn't tested already).

From my understanding (and a bit of code changing on a local version of the library against an app), the change is transparent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants