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.

Sign up for GitHub

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

Jump to bottom

Response factory parameters as named arguments #43248

Merged
merged 10 commits into from Aug 23, 2019
Merged

Response factory parameters as named arguments #43248

merged 10 commits into from Aug 23, 2019

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Aug 14, 2019 •
edited

Summary

There are 2 changes

  1. pass response body as a named parameters in ResponseFactory. as discussed in Unify response interface in handler and request interceptors #42442 (comment)
// before
res.ok('string')
res.redirect(undefined, { headers: { location: '/' } });
// after
res.ok({ body: 'string' })
res.redirect({ headers: { location: '/' } });
  1. pass error details to the client via attributes property of error object. for BWC with Boom error schema.
// before
 return response.badRequest({
   message: 'validation error',
   meta: { data: { ... } }
});
// after
return response.badRequest({
   body: {
     message: 'validation error',
     attributes: { data: { ... } }
   }
});

Dev docs are covered by #41894

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@mshustov mshustov requested review from a team as code owners August 14, 2019 08:12
@mshustov mshustov added Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.4.0 labels Aug 14, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@mshustov mshustov added this to Code review in kibana-core [DEPRECATED] Aug 14, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mshustov mshustov added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. labels Aug 14, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

rudolf
rudolf approved these changes Aug 15, 2019
View reviewed changes
Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

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

Makes the API much nicer :D

src/core/server/http/router/response.ts
@@ -71,7 +69,10 @@ export type HttpResponsePayload = undefined | string | Record<string, any> | Buf
* HTTP response parameters for a response with adjustable status code.
* @public
*/
export interface CustomHttpResponseOptions extends HttpResponseOptions {
export interface CustomHttpResponseOptions<T extends HttpResponsePayload | ResponseError> {
body?: T;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: For completeness sake maybe we should add a doc comment here too

/** HTTP message to send to the client */

src/core/server/server.api.md Outdated
export interface CustomHttpResponseOptions<T extends HttpResponsePayload | ResponseError> {
// (undocumented)
body?: T;
// Warning: (ae-forgotten-export) The symbol "ResponseHeaders" needs to be exported by the entry point index.d.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing type export

src/core/server/server.api.md
// (undocumented)
data?: Record<string, any>;
// (undocumented)
docLink?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

I really liked the idea of typing API best practices like this. It's not critical now, but hopefully we can re-introduce this at some point when we can make breaking changes with HAPI.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@rudolf
Merge branch 'master' of github.com:elastic/kibana into response-sign…
55658c8 
…ature

# Conflicts:
#	docs/development/core/server/kibana-plugin-server.md
#	src/core/server/http/http_server.test.ts
#	src/core/server/http/integration_tests/core_services.test.ts
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@rudolf rudolf merged commit 9a73201 into elastic:master Aug 23, 2019
kibana-core [DEPRECATED] automation moved this from Code review to Needs Backport Aug 23, 2019
@rudolf rudolf mentioned this pull request Aug 23, 2019
Merged
rudolf pushed a commit to rudolf/kibana that referenced this pull request Aug 23, 2019
@mshustov @rudolf
Response factory parameters as named arguments (elastic#43248)
f7a4a90 
* pass body as response parameter. use attributes for error responses

* update core

* update tests

* update x-pack code

* update x-pack tests

* regen docs

* update comment

* Review feedback and fixes after master merge

* Eslint fixes
@joshdover joshdover moved this from Needs Backport to Done (this week) in kibana-core [DEPRECATED] Aug 23, 2019
joshdover pushed a commit that referenced this pull request Aug 23, 2019
@rudolf @joshdover
Response factory parameters as named arguments (#43248) (#43883)
1d7c0d4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kobelb kobelb kobelb approved these changes

@rudolf rudolf rudolf approved these changes

Assignees
No one assigned
Labels
backported Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.4.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mshustov @elasticmachine @kobelb @rudolf