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

[Discuss] Add NP core.http.route #44174

Closed
jfsiii opened this issue Aug 27, 2019 · 10 comments
Closed

[Discuss] Add NP core.http.route #44174

jfsiii opened this issue Aug 27, 2019 · 10 comments
Labels
discuss Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@jfsiii
Copy link
Contributor

jfsiii commented Aug 27, 2019

Bringing this discussion from Slack into an issue

Request: Add a function in NP core.http that behaves like hapi’s server.route i.e. accepts a config object (or array of objects)? It needn't be a 1:1 match with the hapi function. Just something that accepts a config object (or array of them) with the basic info needed (method, path, validate, options, handler, etc)

The Integrations Manager plugin is currently using that approach to define routes.

It's also the method shown in the migration guide

// HTTP functionality from core
core.http.route({ // note: we know routes will be created on core.http
path: '/api/demo_plugin/search',
method: 'POST',
async handler(request) {
const requestFacade: RequestFacade = {
headers: request.headers
};
search(serverFacade, requestFacade);
}
});

This declarative approach is quite nice for authors. They markup the various pieces of the routes, then pass it to a function which does The Right Thing ™️

It also allows a simple data structure to be passed into the plugin which is great for composition and testing.

I think there are many other benefits for both authors and the Platform team, but I'll wait to hear back before going further.

@joshdover joshdover added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Aug 28, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@jfsiii
Copy link
Contributor Author

jfsiii commented Aug 28, 2019

I just saw a recent PR from @elastic/apm-ui which also uses this route+object approach.

@dgieselaar
Copy link
Member

@jfsiii I think you need @elastic/apm-ui (elastic/apm is a little too wide in this case as it includes agents/server).

@mshustov
Copy link
Contributor

I'm not opposed to add proposed API, as it more generic, provides declarative API and supports multiple registration.
Kibana HTTP service is meant to be a thin layer between HTTP server and plugins, so I'm inclining towards providing a single way to declare a route handler. If we want to switch to proposed API it's better to do now till plugins start migration.
@elastic/kibana-platform

@jfsiii
Copy link
Contributor Author

jfsiii commented Aug 29, 2019

I'm happy to submit a PR if that helps. I'd be sure to get agreement on what the interface should be, how to implement, test, etc before starting. Just let me know.

@rudolf
Copy link
Contributor

rudolf commented Sep 3, 2019

@restrry I also like the proposed API and agree that it's better to only expose one way of registering routes.

@joshdover
Copy link
Contributor

I don't have any problems with the proposed API, as long as there's only a single way to register a route, I'm happy.

One slight advantage to the existing API is that we can change the types of the params a bit more easily to better fit the HTTP method. For example, a GET request shouldn't specify any validation schema for it's payload because it should not have one.

In terms of priority, I don't think this is a blocker for anyone, but if there is a strong consensus for the proposed change, then we should do it sooner than later to avoid unnecessary churn.

@jfsiii
Copy link
Contributor Author

jfsiii commented Sep 6, 2019

For example, a GET request shouldn't specify any validation schema for it's payload because it should not have one.

I believe we can use a discriminated union since the different types are all based on the value of the method property.

@mshustov
Copy link
Contributor

mshustov commented Sep 6, 2019

should be done as a part of #44620
I will attach a link to borrow some ideas from the current discussion.

@mshustov mshustov closed this as completed Sep 6, 2019
@jfsiii
Copy link
Contributor Author

jfsiii commented Sep 9, 2019

I don't have any problems with the proposed API, as long as there's only a single way to register a route, I'm happy.

This reminds me of the client-side http interface

  • http.fetch(path, options): This has an API comparable to window.fetch with the addition of a few custom options, status code error handling, default HTTP header additions, and automatic body parsing resolution for JSON and NDJSON.
  • http.get(path, options): The GET-enforced shorthand for http.fetch().
  • http.post(path, options): The POST-enforced shorthand for http.fetch().
  • http.put(path, options): The PUT-enforced shorthand for http.fetch().
  • http.delete(path, options): The DELETE-enforced shorthand for http.fetch().
  • http.patch(path, options): The PATCH-enforced shorthand for http.fetch().

I see router.route as analogous to the client-side http.fetch as the underlying/"longhand" function and router[VERB] functions as the parallels to the http[VERB] shorthands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

6 participants