eugef / node-mocks-http

Mock 'http' objects for testing Express routing functions
Other
747 stars 131 forks source link

Let mock request extend stream.Readable or even http.IncomingMessage #279

Closed hbgl closed 8 months ago

hbgl commented 11 months ago

In an effort to make the mock request behave more like a real http.IncomingMessage, it would make sense to extend from stream.Readable which is the base class of http.IncomingMessage). There has been PR https://github.com/eugef/node-mocks-http/pull/169 that did attempt to extend from stream.Readable but it did not properly implement the Readable protocol which led to issue https://github.com/eugef/node-mocks-http/issues/174.

I also think there is the possibility of inheriting directly from http.IncomingMessage. This would would require the mock to rely on some of Node's implementation details which have been reasonably stable. The big advantage would be that the mock could reuse a large portion of the logic, including the complicated header handling.

@eugef Which of the two options, if any, would you prefer? I would be very happy to work on a PR for this feature.

eugef commented 10 months ago

Which approach is easier to implement and might give less backwards incompatible changes?

hbgl commented 10 months ago

The Readable approach is definitely easier to implement and more backwards compatible than IncomingMessage. One might get away with extending Readable without bumping the major version. Extending IncomingMessage, on the other hand, would be more akin to a rewrite of the mock request.

eugef commented 10 months ago

Ok, then I'd prefer extending from stream.Readable

github-actions[bot] commented 8 months ago

Stale issue message