deno icon indicating copy to clipboard operation
deno copied to clipboard

fix(ext/node): refactor http.ServerResponse into function class

Open nicolabovolato opened this issue 1 year ago • 6 comments

Fixes #19901

Context

light-my-request is used internally by fastify.inject(), which in turn is used to test routes without spinning up an HTTP server.

Since it relied on http.ServerResponse.call() it could not work on Deno, which used an ES6 class. This PR should fix that.

Technical

While testing, I found out that light-my-request relies on ServerResponse.connection, which is deprecated, so I added that and socket, the non deprecated property.

It also relies on an undocumented _header property, apparently for raw header processing. I added it as an empty string, feel free to provide other approaches.

Code is definitely uglier now, but there are not many options... suggestions are welcome!

nicolabovolato avatar Oct 13 '24 16:10 nicolabovolato

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Oct 13 '24 16:10 CLAassistant

@nicolabovolato thanks for the PR. Could you please add a test to tests/unit_node/http_test.ts that tests new APIs you added as well as creating a ServerResponse using "ES5 style classes"?

bartlomieju avatar Oct 13 '24 21:10 bartlomieju

@nicolabovolato thanks for the PR. Could you please add a test to tests/unit_node/http_test.ts that tests new APIs you added as well as creating a ServerResponse using "ES5 style classes"?

@bartlomieju I added some, let me know if we should have more 👍🏼

nicolabovolato avatar Oct 14 '24 08:10 nicolabovolato

@bartlomieju I think I've found abetter way to do this, without es5 classes and without rewriting the whole ServerResponse class.

import { EventEmitter } from "node:events";
import { EventEmitterOptions } from "npm:@types/node";

class Base extends EventEmitter {
  constructor(options?: EventEmitterOptions) {
    super(options);
  }

  static override call(cls: any, options?: EventEmitterOptions) {
    const x = new this(options);
    Object.assign(cls, x);
  }
}

function Class(this: any) {
  Base.call(this, {});
}
Object.setPrototypeOf(Class.prototype, Base.prototype);

const x = new (Class as any)();
console.log(x);

x.on("foo", (data: any) => console.log(data));

x.emit("foo", "bar");

I'll open another pr in the following days.

nicolabovolato avatar Oct 16 '24 19:10 nicolabovolato

@nicolabovolato I think the current solution is fine, it's closer to what Node.js is doing, so I'd prefer to go with this approach.

bartlomieju avatar Oct 16 '24 23:10 bartlomieju

@littledivy please take a look - this is fine for me so if you don't have any objections we should land it.

bartlomieju avatar Oct 22 '24 22:10 bartlomieju