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

refactor(rest): introduce RequestContext #1316

Merged
merged 2 commits into from
May 15, 2018
Merged

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented May 10, 2018

Note: I haven't updated all documentation yet, I am waiting for the outcome of review discussion for the code and API design.

Rework all places where we were accepting a pair of (ParsedRequest, ServerResponse) to use an context object instead.

Two context types are introduced:

HandlerContext is an interface describing objects required to handle an incoming HTTP request. There are two properties for starter:

  • context.request
  • context.response

Low-level entities like Sequence Actions should be using this interface to allow easy interoperability with potentially any HTTP framework and more importantly, to keep the code easy to test.

RequestContext is a class extending our IoC Context and implementing HandlerContext interface. This object is used when invoking the sequence handler.

By combining both IoC and HTTP contexts into a single object, there will be (hopefully) less confusion among LB4 users about what shape a "context" object has in different places.

This is a breaking change affecting the following users:

  • Custom sequence classes
  • Custom sequence handlers registered via app.handler
  • Custom implementations of the built-in sequence action Reject

This is a part of #1292, a follow up for #1082 which supersedes #1313.

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

@bajtos bajtos self-assigned this May 10, 2018
@bajtos bajtos requested review from raymondfeng and hacksparrow May 10, 2018 10:40
@bajtos bajtos added this to the May Milestone milestone May 10, 2018
@bajtos bajtos added refactor REST Issues related to @loopback/rest package and REST transport in general labels May 10, 2018
@bajtos
Copy link
Member Author

bajtos commented May 10, 2018

Few notes for reviewers:

Let's work incrementally. This pull request is focused on changing everything Sequence related from (req, res) to {req, res} style. The actual HTTP-processing layer remains unchanged for now. I am open to apply similar changes there too, but not in this pull request.

@raymondfeng let me answer your concerns from #1292 (comment) here.

  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.

I agree. To address this problem, I am introducing two types:

  • HandlerContext (maybe we should rename it to HttpHandlerContext?) as a low-level object containing only HTTP entities like request and response. In my vision, this low-level interface could be used at the point where our Sequence-based router integrates with an HTTP framework like Express, if desired.

  • RequestContext is a combination of IoC Context and HandlerContext. My feeling is that a single context object can simplify many places around the sequence. For example, in the app.handler function, we don't need to distinguish between a handler context containing request/response and an IoC context previously available via sequence.ctx - there is a single context argument providing access to both now.

  1. We can use getter function for req/res so that we can always get the bound req/res.

I am worried about the performance impacts of running full context/binding resolution whenever we need to access the request or response object. For now, I am using a regular property and a locked binding to prevent people from overriding request/response bound in the context.

Either way, the public API remains the same, therefore we can always easily change the implementation details if/when we have a use case requiring it. I am proposing to keep my current version for this initial pull request.

@hacksparrow
Copy link
Contributor

I like the names HandlerContext and RequestContext . Unless there are going to be more handlers, we should stick with HandlerContext, since HTTP is the context.

@raymondfeng
Copy link
Contributor

@bajtos Please fix the code lint errors.

@bajtos
Copy link
Member Author

bajtos commented May 10, 2018

Please fix the code lint errors.

Weird. npm run lint was passing locally for me, I'll try to reinstall all dependencies.

@raymondfeng do you have any more feedback/review comments?

@bajtos bajtos force-pushed the refactor/http-req-res branch from c4b0f71 to d1eb28e Compare May 10, 2018 14:33
Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

On a high level, this LGTM. Personally, I like the simplification of merging the IoC context and request/response into RequestContext. I will do another round of review later for more thoughtful feedback.

The types defined in "internal-types.ts" are not internal at all,
for example they include signatures of sequence actions.

This commit renames the file to "types.ts", which is a file name
already used in other packages (let's be consistent).
@bajtos bajtos force-pushed the refactor/http-req-res branch from d1eb28e to e8f51e2 Compare May 15, 2018 08:43
Rework all places where we were accepting a pair of
`(ParsedRequest, ServerResponse)` to use an context object instead.

Two context types are introduced:

`HandlerContext` is an interface describing objects required to
handle an incoming HTTP request. There are two properties for starter:
  - context.request
  - context.response

Low-level entities like Sequence Actions should be using this
interface to allow easy interoperability with potentially any
HTTP framework and more importantly, to keep the code easy to test.

`RequestContext` is a class extending our IoC `Context` and
implementing `HandlerContext` interface. This object is used
when invoking the sequence handler.

By combining both IoC and HTTP contexts into a single object,
there will be (hopefully) less confusion among LB4 users about
what shape a "context" object has in different places.

This is a breaking change affecting the following users:

- Custom sequence classes
- Custom sequence handlers registered via `app.handler`
- Custom implementations of the built-in sequence action `Reject`
@bajtos bajtos force-pushed the refactor/http-req-res branch from e8f51e2 to c709571 Compare May 15, 2018 10:03
@bajtos bajtos merged commit b56ad66 into master May 15, 2018
@bajtos bajtos deleted the refactor/http-req-res branch May 15, 2018 10:30
@dhmlau dhmlau mentioned this pull request May 17, 2018
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor REST Issues related to @loopback/rest package and REST transport in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants