chaijs / chai-http

HTTP Response assertions for the Chai Assertion Library.
http://chaijs.com/plugins/chai-http
634 stars 113 forks source link

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

Open J4Numbers opened 11 months ago

J4Numbers commented 11 months ago

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:

J4Numbers commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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.