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

feat(rest): expose app.requestHandler function #1084

Merged
merged 1 commit into from
Mar 16, 2018

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Mar 5, 2018

Each RestServer provides an HTTP handler function that can be used with core HTTP server or any compatible framework like Express.

This pull request exposes the handler function on the RestApplication class to simplify the usage.

See also:

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • Related API Documentation was updated
  • (n/a) Affected artifact templates in packages/cli were updated
  • Affected example projects in packages/example-* were updated

@bajtos bajtos added developer-experience Issues affecting ease of use and overall experience of LB users feature REST Issues related to @loopback/rest package and REST transport in general labels Mar 5, 2018
@bajtos bajtos added this to the March 2018 milestone Mar 5, 2018
@bajtos bajtos requested review from virkt25 and shimks March 5, 2018 13:07
@bajtos
Copy link
Member Author

bajtos commented Mar 5, 2018

I'd like to get few more approvals before merging this pull request. Since I'll be away from the computer for the next 10 days, could somebody from the approvers land the pull request for me please?

@virkt25
Copy link
Contributor

virkt25 commented Mar 11, 2018

Code looks good BUT I would like for us to reconsider the name for this function. handleHttp sounds odd and the the intent isn't clear / obvious. Some alternate proposals:

  • requestHandler
  • httpHandler (this is already in use though)
  • handleHttpReq(uest)

@bajtos
Copy link
Member Author

bajtos commented Mar 16, 2018

@virkt25 thank you for a thoughtful comment.

Let's review all different request-handling members we have in RestServer so that we can find the best new name.

  • handleHttp is the handler function to pass to a core HTTP server instance, an Express app, etc. Right now, it's a simple wrapper around _handleHttpRequest, preserving this and providing error handling.
  • _handleHttpRequest is the method which contains the core of request handling code.
  • httpHandler is an instance of HttpHandler class, it deals with routing and Sequence invocation

In this light, I feel your proposals httpHandler, handleHttpReq and handleHttpRequest would only add more confusion.

What I dislike about requestHandler:

  • Functions/method names should start with a verb.
  • I'd like to include "http" in the method name, because an application can have request handlers for different protocols too.

On the other hand, we are not using requestHandler as a method, but we are passing it around as an object (something that can handle requests). Also if we are accessing this property (typically to pass it down to an HTTP server or Supertest), then we must already know we are dealing with HTTP handler, thus I guess it's ok to leave "http" out?

Initially, I wanted the RestApplication to use the same name as RestServer for this HTTP handler function. If I go with your proposal to use requestHandler as the name, then I would like to rename RestServer#handleHttp for consistency too. Is this fine with you?

@bajtos bajtos force-pushed the feat/rest-app-handle-http branch 2 times, most recently from 18fa3d3 to aa5f5c5 Compare March 16, 2018 13:45
@bajtos
Copy link
Member Author

bajtos commented Mar 16, 2018

Performed the rename in aa5f5c5.

@virkt25 LGTY now?

Copy link
Contributor

@virkt25 virkt25 left a comment

Choose a reason for hiding this comment

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

👏 I like the requestHandler much much much better than httpHandler 👍

Each RestServer provides an HTTP handler function that can be used
with the core HTTP server or any compatible framework like Express.

This change exposes the handler function on the RestApplication class
to simplify the usage.

BREAKING CHANGE: `RestServer#handleHttp` was renamed to
`RestServer#requestHandler`.
@bajtos bajtos changed the title feat(rest): expose app.handleHttp() function feat(rest): expose app.requestHandler function Mar 16, 2018
@bajtos bajtos merged commit 20a41ac into master Mar 16, 2018
@bajtos bajtos deleted the feat/rest-app-handle-http branch March 16, 2018 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Issues affecting ease of use and overall experience of LB users feature 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.

3 participants