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 inheritance should not overwrite the methods on res and req with the ones from http.ServerResponse and http.IncomingMessage #6047

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Tofandel
Copy link

@Tofandel Tofandel commented Oct 13, 2024

This allows for express to run correctly with unenv and paves the way for it to be used in other envs than node

Fixes #6039

I also have a backport for 4.x ready

@Tofandel Tofandel changed the title Fix inheritance should not overwrite the methods res and req with the ones from http.ServerResponse and http.IncomingMessage Fix inheritance should not overwrite the methods on res and req with the ones from http.ServerResponse and http.IncomingMessage Oct 13, 2024
@wesleytodd
Copy link
Member

Unfortunately I think we are lacking a few things on this PR:

  1. A clear description of what you are trying to achieve: Please explain in detail what you are trying to achieve here. If that is what is outlined in Response built by express is broken if res and req don't extend node's http.IncomingMessage and http.ServerResponse #6039 then go ahead and just close that and copy over the content into this description. But having read that issue I still think we need to clarify the goals because your follow up comment in there is really not something I think which aligns with our future goals...which leads me to:
  2. Alignment with our future plans: We will be fully replacing the old and bad patching express does for v6. Decoupling from node pass through api's and monkey patching is a goal. I am not sure it makes sense to increase complexity in this way for the older lines we will need to maintain for a few years knowing we will be making larger changes anyway going forward.
  3. Cleanup: maybe it was intended to be an MVP, but for sure we would need to do some cleanup if our goal is to try and fix the old coupling. Maybe this is a thing with unenv (which I dont personally use, although have looked at a few times), but I dont think the project is interested in doing partial measures to support new environments. I think we are more interested in making the right changes to adopt well specified and adopted apis which do not need polyfills.

If you are interested in working toward this same goal then I would love to chat more on this. I am just not sure I see how this specific PR fits in with the plans.

@Tofandel
Copy link
Author

Tofandel commented Jan 9, 2025

Yes this is what is outlined in #6039

The reason for using unenv is just to be able to run SSR requests/fetch on the same server without a network roundtrip. Basically on nuxt SSR, unenv is there so that when you fetch locally a mocked Request is sent to express.

The problem this is solving is the use of setPrototypeOf overrides all of the existing methods on the request and response objects with the native ones from the node class, not just the ones defined by express and added by users of express but all the native api methods as well

As such if you have a response or request object not originally coming from node, but simply meant to have a compatible api (like unenv) the methods of the unenv class will be replaced with the methods of the node class but the internals of the class very much differ, resulting in errors like with getHeaders in the issue

This PR fixes that by running a modified setPrototypeOf that will only replace all the non native methods of the class (so what express is adding and the users are adding, but not the ones from the original prototype)

I do not get your point about this PR needing cleanup, it's a pretty straightforward PR with minimal changes for a problem that took me weeks if not months to track down and solve efficiently without breaking anything in express. It has clearly already been cleaned up

We will be fully replacing the old and bad patching express does for v6

Express is being considered legacy because of those design issues. Meaning if I do want to use express on h3/unenv, it's because I need a few endpoints coming from an (oldish) library built for express. Without having to redevelop them or having to wait for it to become compatible with express v6 which would likely take a few years after v6 comes out as it is still on express v4.

So while this is really great news, it's already a bit late and v5 which is pretty recent should get some attention to make it compatible with other envs if it's doable minimally and without breaking change like here

@wesleytodd
Copy link
Member

As such if you have a response or request object not originally coming from node, but simply meant to have a compatible api (like unenv) the methods of the unenv class will be replaced with the methods of the node class but the internals of the class very much differ, resulting in errors like with getHeaders in the issue

If you provide some code examples it might help, but I think you can use the app.request and app.response to override the base classes already. Those are what is used in the code you changed as the base.

https://github.com/expressjs/express/blob/master/lib/express.js#L45-L52

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement semver-minor This change is a semver minor
Projects
None yet
4 participants