Change (req, res) pair to a single http context object

[hacksparrow]

Description / Steps to reproduce / Feature proposal

Use a single object object to contain the request and response object.

Current Behavior

(req, res)

Expected Behavior

(httpContext)

Acceptance Criteria

(req, res ...) should be replaced with (httpContext ...).

See Reporting Issues for more tips on writing good issues

[bajtos]

We should consider a different name than httpContext - I am worried "context" is to easy to confuse with IoC Context class. See #1082 (comment):

Using the name "context" for the object containing HTTP request/response is confusing, because we also have Context class with app-level and request-level instances. Let's find a better, less ambiguous name please!

[bajtos]

Cross-posting from a private chat:

Any suggestion for the { req, res } object name?

Hmm, not really 😞 Maybe HttpFrame or HttpRequestFrame? It is a bit unusual though.

As I am thinking about this problem, I am leaning towards the conclusion that the problem is in the very fact that we have two request-related contexts: one holding req/res objects, another an instance of Context.

What if we reworked our design so that there is only a single http-request-related context object?

For example:

class HttpRequestContext extends Context {
  constructor(
    parentContext: Context, 
    public readonly request: ExpressRequest,
    public readonly response: ExpressResponse,
    // add any other required args/properties as needed) {
  ) {
    super(parentContext);
    this.bind(RestBindings.Http.REQUEST).to(this.request).lock();
    this.bind(RestBindings.Http.RESPONSE).to(this.response).lock();
    //...
  }
}

The upside is that we remove any possible confusion about what kind of an object we are receiving as a context and any code working at this low level automatically receive access to the IoC context associated with the incoming request.

The potential downside is that we make it easier for low-level HTTP code to bypass dependency injection via @inject and use the context as a Service Locator, which is sort of an anti-pattern. Having said that, one can always get hold of the current context via @inject.context(), thus my proposal will not make things much worse.

@raymondfeng @hacksparrow thoughts?

[hacksparrow]

Would like to hear from @raymondfeng.

[raymondfeng]

I like @bajtos's proposal to consolidate the http req/res wrapper object with our IoC Context. But I do have some concerns too:

1. HttpRequestContext becomes more complex that just a wrapper of req/res.
2. The construction of HttpRequestContext needs collaboration from LB and the underlying http framework. In my original PoC, I expect the http framework to create the HttpContext.
3. We can use getter function for req/res so that we can always get the bound req/res.

[bajtos]

I do have some concerns too.

Responded in my pull request, let's keep the discussion there for now: #1316 (comment)

[bajtos]

Closing as done.