Extract useRequest and sendRequest from SR into elasticsearch-ui-shared plugin by cjcenizal · Pull Request #40777 · elastic/kibana

cjcenizal · 2019-07-10T15:58:06Z

Note to reviewers: I tried to structure my commit history in a way that makes my changes easy to follower. If the changes in this PR seem too disparate to digest I recommend reviewing each commit individually.

Changes to useRequest:

I extracted the sendRequest function and useRequest hook used by SR into an elasticsearch-ui-shared plugin (based off Lib "Hook form" #39347).
Consumers are now required to provide an http service as an argument to sendRequest and useRequest.
I refined the useRequest contract a bit, most notably removing the polling flag from the return value and adding an isInitialRequest flag in its place. I also removed the changeInterval function from the return value so that consumers are expected to change the interval config parameter instead.
I also uncoupled sendRequest and useRequest from the concept of telemetry, by requiring consumers of sendRequest to do their own telemetry logic inside of the returned promise's success callback. I think this is more flexible because this is the logical place for consumers to perform any actions once the request has succeeded. I added a processData parameter to the useRequest hook which is intended for processing the data resolved by a request, but it can also support similar telemetry functionality or other side effects that need to occur after a request has successfully resolved.

I also simplified the http and uiMetric services in SR by exporting methods directly from these modules, instead of exporting a namespace object. This reduced some boilerplate on the consumer's side. @jen-huang How do these changes look to you as the original author?

elasticmachine · 2019-07-10T15:58:08Z

Pinging @elastic/es-ui

x-pack/legacy/plugins/snapshot_restore/public/app/services/http/repository_requests.ts

elasticmachine · 2019-07-10T17:36:23Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-10T18:17:52Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-10T19:26:48Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-10T23:25:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-12T01:57:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…ed plugin. - Refactor SR's consumption to send UI metric telemetry in the promise chain, to uncouple the request functions from telemetry.

…lling state.

- Rename loading flag to isLoading. - Rename changeInterval callback to setRequestInterval. - Remove initialData from return value. - Preserve complete response shape instead of removing everything except the data property. - Send a request when path, body, or method changes. - Don't send a request when the interval changes.

elasticmachine · 2019-07-13T01:11:53Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

alisonelizabeth · 2019-07-25T11:50:40Z

@cjcenizal did another quick test of watcher with your latest changes and did not find any issues.

- Remove unnecessary kibana.json and no-op plugin definition. - Remove root-level index file because it's a scalability bottleneck. - Move request module into its own directory.

elasticmachine · 2019-07-25T17:06:38Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal · 2019-07-25T17:23:45Z

@sebelga Here's the new folder structure, updated with the same pattern as used by the kibana_react plugin (which serves a similar purpose to es_ui_shared). I think this will scale well and make it clear where code can be used. My only worry is that we're losing something by not incorporating a static directory here, but I'll look to you to help me understand that better.

jen-huang

Did another local test of SR and didn't find any regressions. LGTM!

elasticmachine · 2019-07-25T18:12:09Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-25T19:30:21Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal · 2019-07-26T04:10:29Z

Retest

elasticmachine · 2019-07-26T05:14:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

sebelga · 2019-07-29T10:20:10Z

Thanks for making the changes @cjcenizal ! I added my comment about the implicit scheduler so you see where I'm coming from.

I am not sure about the folder structure, I keep thinking that what we are doing is a hack around the "we don't know where to put static code" problem. To me, public and server are reserved folders for plugins that get started (start()) by the Core plugin.

As an example: the "mappings editor" is a reusable component that belongs to the Index management plugin. But it will be consumed by other plugins (ML). As a reusable static code, I made it available from the "static" folder of "index_management". It is now clear that we have a contract with others and we have to take into consideration breaking changes on the input/output. The piece of the puzzle that is missing (and is easy to solve) is a way for plugins to declare their dependency on other plugins static code.
We can talk about this over Zoom better.

Everything will eventually work, this is just to make things more explicit. 😊

elasticmachine · 2019-07-29T17:24:40Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-29T19:18:04Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal · 2019-07-29T19:26:48Z

Retest

cjcenizal · 2019-07-29T19:34:02Z

Retest

elasticmachine · 2019-07-29T20:38:39Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cjcenizal · 2019-07-29T21:26:08Z

I spoke with @sebelga on Slack and he agreed that we could merge this PR and discuss the other points he brought up later. I'll leave my thoughts here for now.

I see useRequest as a hooked http client to make HTTP requests in a component and get a Response from the server.

Critiquing pollInterval from this point of view makes sense to me. I have a different perspective on useRequest. I see it as a tool we're building for general use within our apps, so I'd like it to address common needs beyond those of an http client, polling being one of them. In terms of interface, I'm happy with requiring the consumer to explicitly define pollInterval as a value to enable polling, or leaving it as undefined to leave polling disabled. I can change my mind on this if we notice problems with people consuming useRequest and experiencing confusion with the interface.

To me, public and server are reserved folders for plugins that get started (start()) by the Core plugin.

This sounds like a good question for the Platform team.

As a reusable static code, I made it available from the "static" folder of "index_management".

Makes sense. I'm open to working through this more!

…lastic#40777) - Refactor SR's consumption to send UI metric telemetry in the promise chain, to uncouple the request functions from telemetry. - Refactor useRequest to return isInitialRequest instead of tracking polling state. - Rename request callback to sendRequest. - Rename loading flag to isLoading. - Rename changeInterval callback to setRequestInterval. - Rename interval parameter to pollIntervalMs. - Remove initialData from return value. - Preserve complete response shape instead of removing everything except the data property. - Send a request when path, body, or method changes. - Don't send a request when the interval changes. - Remove setRequestInterval from return value. - Expect the consumer to change the interval config in order to set it to a new value. - Refactor internals so that calling sendRequest resets the interval. - Migrate Watcher to use shared request library. - Rename onSuccess to deserializer and use it to process data. - Rename updateInterval function to scheduleRequest. - Don’t watch method parameter.

…40777) (#42200) - Refactor SR's consumption to send UI metric telemetry in the promise chain, to uncouple the request functions from telemetry. - Refactor useRequest to return isInitialRequest instead of tracking polling state. - Rename request callback to sendRequest. - Rename loading flag to isLoading. - Rename changeInterval callback to setRequestInterval. - Rename interval parameter to pollIntervalMs. - Remove initialData from return value. - Preserve complete response shape instead of removing everything except the data property. - Send a request when path, body, or method changes. - Don't send a request when the interval changes. - Remove setRequestInterval from return value. - Expect the consumer to change the interval config in order to set it to a new value. - Refactor internals so that calling sendRequest resets the interval. - Migrate Watcher to use shared request library. - Rename onSuccess to deserializer and use it to process data. - Rename updateInterval function to scheduleRequest. - Don’t watch method parameter.

sebelga · 2019-07-30T08:58:07Z

Note for myself and future discussion:

Another reason why "static" is better than "public" and "server" is that it does not make a difference where the code will be consumed (server or client). When we will put the AuthorizationProvider code in the "es_ui_shared" we will be forced to put it in the "public". Even though the server code also consumes the typings.

Instead of "static", we can think of "packages" or "node_modules".

This sounds like a good question for the Platform team.

I thought we were going to solve this at our team level and inspire others 😊

cjcenizal added v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// release_note:skip Skip the PR/issue when compiling release notes v7.4.0 labels Jul 10, 2019

cjcenizal force-pushed the shared/use-request branch from 3b50a35 to 0c1e55f Compare July 10, 2019 16:06

cjcenizal commented Jul 10, 2019

View reviewed changes

x-pack/legacy/plugins/snapshot_restore/public/app/services/http/repository_requests.ts Outdated

This comment was marked as resolved.

Sign in to view

cjcenizal force-pushed the shared/use-request branch 2 times, most recently from 0566dc6 to 76b5f1d Compare July 10, 2019 18:17

alisonelizabeth mentioned this pull request Jul 10, 2019

[IM] Index Templates UI list view #39922

Merged

cjcenizal added the WIP Work in progress label Jul 10, 2019

cjcenizal force-pushed the shared/use-request branch from aa50d19 to 4921550 Compare July 10, 2019 22:20

cjcenizal added 7 commits July 12, 2019 17:03

Extract useRequest and sendRequest from SR into elasticsearch-ui-shar…

e4f0d5e

…ed plugin. - Refactor SR's consumption to send UI metric telemetry in the promise chain, to uncouple the request functions from telemetry.

Simplify http service to reduce boilerplate.

b9765d3

Simplify uiMetric service to reduce boilerplate.

8fea574

Refactor useRequest to return isInitialRequest instead of tracking po…

6bf33ec

…lling state.

Rename request callback to sendRequest.

8acddae

Add test coverage.

1254d38

cjcenizal force-pushed the shared/use-request branch 2 times, most recently from 2f661fd to 38c0be4 Compare July 13, 2019 00:16

cjcenizal requested review from alisonelizabeth, jen-huang and sebelga July 13, 2019 00:17

Refator folder structure.

02e38e5

- Remove unnecessary kibana.json and no-op plugin definition. - Remove root-level index file because it's a scalability bottleneck. - Move request module into its own directory.

Fix SR regression caused by merge resolution.

9cf694d

jen-huang approved these changes Jul 25, 2019

View reviewed changes

Merge branch 'master' into shared/use-request

6abb68c

Merge branch 'master' into shared/use-request

89d06f9

cjcenizal added 2 commits July 29, 2019 08:40

Merge branch 'master' into shared/use-request

68376ae

Make deserializer type clearer.

9e91d25

Merge branch 'master' into shared/use-request

fec0956

cjcenizal merged commit c91e945 into elastic:master Jul 29, 2019

cjcenizal deleted the shared/use-request branch July 29, 2019 21:28

cjcenizal mentioned this pull request Jul 29, 2019

[7.x] Extract useRequest and sendRequest from SR into es-ui-shared plugin (#40777) #42200

Merged

cjcenizal mentioned this pull request Jul 30, 2019

Disable request module's reset pollInterval test. #42259

Closed

alisonelizabeth mentioned this pull request Jan 16, 2020

Watcher UI hangs when navigating to a watch that doesn't exist. #51138

Closed

Conversation

cjcenizal commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jul 10, 2019

Uh oh!

This comment was marked as resolved.

Uh oh!

elasticmachine commented Jul 10, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jul 10, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jul 10, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jul 10, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Jul 12, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Jul 13, 2019

💔 Build Failed

Uh oh!

alisonelizabeth commented Jul 25, 2019

Uh oh!

elasticmachine commented Jul 25, 2019

💔 Build Failed

Uh oh!

cjcenizal commented Jul 25, 2019

Uh oh!

jen-huang left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 25, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jul 25, 2019

💔 Build Failed

Uh oh!

cjcenizal commented Jul 26, 2019

Uh oh!

elasticmachine commented Jul 26, 2019

💚 Build Succeeded

Uh oh!

sebelga commented Jul 29, 2019

Uh oh!

elasticmachine commented Jul 29, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jul 29, 2019

💔 Build Failed

Uh oh!

cjcenizal commented Jul 29, 2019

Uh oh!

cjcenizal commented Jul 29, 2019

Uh oh!

elasticmachine commented Jul 29, 2019

💚 Build Succeeded

Uh oh!

cjcenizal commented Jul 29, 2019

Uh oh!

sebelga commented Jul 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cjcenizal commented Jul 10, 2019 •

edited

Loading