Skip to content

Expose an HTTP-request browser client#35486

Merged
eliperelman merged 15 commits intoelastic:masterfrom
eliperelman:browser-http-service
May 8, 2019
Merged

Expose an HTTP-request browser client#35486
eliperelman merged 15 commits intoelastic:masterfrom
eliperelman:browser-http-service

Conversation

@eliperelman
Copy link
Copy Markdown
Contributor

@eliperelman eliperelman commented Apr 23, 2019

Summary

This patch exposes a browser client for simplifying HTTP requests. The long-term goal of this service/client is to replace the legacy kfetch service. This patch also swaps out the internals of kfetch for this new service, meaning legacy and NP plugins are both using this.

From a sat up HttpService, there are now 5 additional methods:

  • 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().

Fixes #18855.

Checklist

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

For maintainers

Dev Docs

The HTTP browser client now exposes methods for simplifying HTTP requests. The long-term goal of this service/client is to replace the legacy kfetch service. This also swaps out the internals of kfetch for this new service, meaning legacy and new platform plugins are both backed by this functionality.

From a sat up HttpService, there are now 5 additional methods:

  • 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().

@eliperelman eliperelman added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Feature:New Platform v8.0.0 v7.2.0 labels Apr 23, 2019
@eliperelman eliperelman requested a review from a team as a code owner April 23, 2019 16:38
@eliperelman eliperelman self-assigned this Apr 23, 2019
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-platform

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@eliperelman eliperelman force-pushed the browser-http-service branch 2 times, most recently from 838b2b2 to fff35e0 Compare April 23, 2019 19:55
@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@eliperelman eliperelman force-pushed the browser-http-service branch 2 times, most recently from ab8fe91 to 32af0ba Compare April 24, 2019 16:27
@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I didn't use it because when you call .mockReturnValue(createSetupContractMock()), typescript doesn't infer type to HttpService.setup yet.
you can pass .mockReturnValue({}), and it won't complain.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I didn't use it

Could you elaborate on what you mean by this?

Copy link
Copy Markdown
Contributor

@mshustov mshustov Apr 26, 2019

Choose a reason for hiding this comment

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

that's why I prefer next form:

// here we infer type manually
const startContract: StartContract = {
  setup: jest.fn()
}
// now we have proper validation
startContract.mockReturnValue(..)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is currently inconsistent with the way the rest of the client-side mocks work, so I'm going to leave the current form. We can re-evaluate this again in the future.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, now I know why you did it this way, @restrry. I think having the type-safety is preferable and we should probably change the ones I had changed back.

@eliperelman eliperelman force-pushed the browser-http-service branch from a02bda7 to 0e86ce1 Compare April 25, 2019 19:40
@tylersmalley
Copy link
Copy Markdown
Member

@mistic, would you mind taking a look at this error? Additionally, we need to come up with a way for folks to be able to run the typechecks performed in the build task locally. Currently, it's isolated to the build.

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@eliperelman
Copy link
Copy Markdown
Contributor Author

According to @spalger this was due to kfetch_test_setup.ts being included in the build when it shouldn't have been. I resolved that particular issue by moving it to test_utils.

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@mistic
Copy link
Copy Markdown
Contributor

mistic commented Apr 26, 2019

During the build the typecheck we did against the built assets are basically tsc --noEmit true --pretty true --project ${KIBANA_BASE_DIR}/build/kibana-oss/tsconfig.browser.json which is basically the same as we did with node scripts/type_check with the different that the first one is applied against the built assets. I think the problem here was the one highlighted by @spalger

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@eliperelman eliperelman force-pushed the browser-http-service branch from 6fe3a69 to e7c48ff Compare April 30, 2019 16:13
@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@eliperelman
Copy link
Copy Markdown
Contributor Author

retest

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@eliperelman eliperelman force-pushed the browser-http-service branch from e7c48ff to b2e04a4 Compare April 30, 2019 19:45
@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@epixa epixa added the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label May 8, 2019
@eliperelman eliperelman force-pushed the browser-http-service branch from 51b1883 to e3db5be Compare May 8, 2019 14:40
@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@eliperelman eliperelman merged commit 95a3ceb into elastic:master May 8, 2019
eliperelman added a commit to eliperelman/kibana that referenced this pull request May 10, 2019
* Expose an HTTP-request browser client

* Fix failing tests from kfetch refactor

* Make abort() non-enumerable, fix review issues

* Move kfetch test setup to build-excluded location

* Add ndjson tests to browser http service

* Lint fixes

* Fix missing update of del to delete in http mock

* Fix problems with merging headers with undefined Content-Type

* Delete correct property from updated options

* Linting fix

* Fix reference to kfetch_test_setup due to moving test file

* Add tests and fix implementation of abortables

* Add missing http start mock contract, fix test in CI

* Remove abortable promise functionality

* Fix DELETE method handler, remove unnecessary promise wrapper
eliperelman added a commit that referenced this pull request May 10, 2019
* Expose an HTTP-request browser client

* Fix failing tests from kfetch refactor

* Make abort() non-enumerable, fix review issues

* Move kfetch test setup to build-excluded location

* Add ndjson tests to browser http service

* Lint fixes

* Fix missing update of del to delete in http mock

* Fix problems with merging headers with undefined Content-Type

* Delete correct property from updated options

* Linting fix

* Fix reference to kfetch_test_setup due to moving test file

* Add tests and fix implementation of abortables

* Add missing http start mock contract, fix test in CI

* Remove abortable promise functionality

* Fix DELETE method handler, remove unnecessary promise wrapper
@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v7.2.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[browser] HTTP service

8 participants