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

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

Merged

Conversation

nicolabovolato
Copy link
Contributor

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!

@CLAassistant
Copy link

CLAassistant commented Oct 13, 2024

CLA assistant check
All committers have signed the CLA.

@nicolabovolato nicolabovolato force-pushed the fix/node-http-server-response branch from a34d2f0 to 5240833 Compare October 13, 2024 18:43
@bartlomieju
Copy link
Member

@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"?

Fixes denoland#19901

Added socket, connection and _header params
@nicolabovolato nicolabovolato force-pushed the fix/node-http-server-response branch from 5240833 to a1d825d Compare October 14, 2024 08:51
@nicolabovolato
Copy link
Contributor Author

nicolabovolato commented Oct 14, 2024

@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
Copy link
Contributor Author

@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.

@bartlomieju
Copy link
Member

@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 bartlomieju self-requested a review October 16, 2024 23:23
@bartlomieju
Copy link
Member

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

Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bartlomieju bartlomieju merged commit 8dd6177 into denoland:main Oct 24, 2024
17 checks passed
bartlomieju added a commit that referenced this pull request Oct 25, 2024
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](https://github.com/fastify/light-my-request/blob/v6.1.0/lib/response.js#L180-L186).
I added it as an empty string, feel free to provide other approaches.

Fixes #19901

Co-authored-by: Bartek Iwańczuk <biwanczuk@gmail.com>
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.

Package compatibility problem with light-my-request
4 participants