Fix es_ui_shared eslint violations for useRequest hook #72947

cjcenizal · 2020-07-22T19:33:30Z

Replaces #59626

Partially addresses #49554

Closes #49572 (originally fixed as part of the NP migration)
Closes #49562 (originally fixed as part of the NP migration)
Closes #42225 (flaky test)
Closes #42563 (flaky test)
Closes #42562 (flaky test)
Closes #42561 (flaky test)

This PR:

Removes the unused original request.ts helper and replaces it with the NP-ready version
~~Adds clean-up logic for when the component using this helper is unmounted~~ (This was reverted)
Unskips the tests and rewrites them to run synchronously using fake timers.
Converts the tests to TypeScript.
Relaxes the SendRequestResponse type definition to type error as any instead of Error, since we weren't actually fulfilling that type.

To test:

Verify that requests behave as expected in Index Management, Watcher, Snapshot Restore, Ingest Manager, and Transforms

Errors

I tested error-handling using data streams.

ES errors

405

400 `caused_by` chain

I was concerned that the change in the way sendRequest surfaced errors would change the way caused_by chains were being surfaced in the UI, but it looks like the two apps that use this module and surface caused_by chains actually return them as part of the success response, not the error response. These apps are Snapshot Restore (deletRepositories endpoint) and Index Management (deleteDataStreams and deleteIndexTemplates endpoints).

Non-ES errors

500

elasticmachine · 2020-07-31T16:54:52Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

- Add clearer cleanup logic for unmounted components. - Align logic and comments in np_ready_request.ts and original request.ts.

…o false during cleanups unrelated to unmounting.

…val.

- Split request.ts into send_request.ts and use_request.ts. - Convert test files into TS. - Relax SendRequestResponse type definition to type error as any instead of expecting an Error, since we weren't actually fulfilling this expectation.

cjcenizal · 2020-08-01T17:51:29Z

src/plugins/es_ui_shared/public/request/use_request.ts

    // To be functionally correct we'd send a new request if the method, path, query or body changes.
    // But it doesn't seem likely that the method will change and body is likely to be a new
    // object even if its shape hasn't changed, so for now we're just watching the path and the query.
-  }, [path, stringifiedQuery]);
+  }, [path, query]);


These are the only dependencies that I think should have any effect on the hook behavior, at least at this point in this hook's development. If we were to change the refs above to useCallback, I'm concerned about introducing new and possibly unwanted behavior. For example, converting sendRequestRef to useCallback means defining dependencies that duplicate those defined here (see below). I think this makes the code's behavior more difficult to predict from reading it and I'm not clear on what the benefit would be.

I'm open to exploring this in separate PRs, but I think for the context of this PR I'd like to avoid introducing any complexity that doesn't have a clear, deliberate purpose (e.g. fixing a bug or defining behavior more clearly). I think the current code makes it clear what will happen if a) dependencies change and b) sendRequestRef is called. If other people feel the same way then I think that's enough for us to consider this merge-worthy in its current form.

const sendRequest = useCallback(async () => { // We don't clear error or data, so it's up to the consumer to decide whether to display the // "old" error/data or loading state when a new request is in-flight. setIsLoading(true); const requestBody = { path, method, query, body, }; const response = await sendRequest<D, E>(httpClient, requestBody); const { data: serializedResponseData, error: responseError } = response; // If an outdated request has resolved, ignore its outdated data. if (isOutdatedRequest) { return { data: null, error: null }; } setError(responseError); // If there's an error, keep the data from the last request in case it's still useful to the user. if (!responseError) { const responseData = deserializer(serializedResponseData); setData(responseData); } setIsLoading(false); setIsInitialRequest(false); // If we're on an interval, we need to schedule the next request. This also allows us to reset // the interval if the user has manually requested the data, to avoid doubled-up requests. scheduleRequestRef.current!(); return { data: serializedResponseData, error: responseError }; }, [body, deserializer, httpClient, isOutdatedRequest, method, path, query]);

cjcenizal · 2020-08-03T18:32:02Z

CC @walterra @jen-huang. The code here is the same as what you approved in #59626. I'm tagging you for visibility, and I intend to merge this once @sebelga approves. We have most of the release cycle to detect any bugs introduced into the consuming apps by these changes.

One subtle change that could cause a change in behavior is in the way sendRequest returns errors. It changed from this:

return {
  data: null,
  error: e.response ? e.response : e,
};

To this:

return {
  data: null,
  error: e.response && e.response.data ? e.response.data : e.body,
};

To find and fix any bugs means we'll need to look through Snapshot Restore and Ingest Manager, which depended upon the old behavior.

This reverts commit 2fe8e19.

cjcenizal · 2020-08-04T23:08:01Z

@sebelga Thanks for catching that bug! I accidentally introduced that in 2fe8e19, so I reverted the commit and added a comment to explain that line's role.

…ed on its dependencies. - Update sendRequest to clear the poll interval immediately when it's called. - Update eslintrc to lint es_ui_shared. - Type arguments correctly as UseRequestConfig in helpers.

… add test coverage for request behavior.

cjcenizal · 2020-08-29T23:55:23Z

src/plugins/es_ui_shared/public/request/use_request.ts

+    if (!isLoading) {
+      scheduleRequest();
+    }
+  }, [isLoading, scheduleRequest]);


Note that here we're using isLoading to signal the need to reschedule a request, and not totalRequests as in https://github.com/cjcenizal/kibana/pull/25/files#diff-a60f7c6c34d3d037970d34856d1b3da2R142.

I think it makes sense to keep each hook as single-purpose as possible. This hook's sole purpose is to manage the polling rhythm.

I think it is a little bit less explicit than with my totalRequest count (I personally understand better "when the request count increments, reschedule a request", as "isLoading" seems like it could change independently.) but I am OK to go with your suggestion.

But your comment is not right, the reschedule will happen whenever scheduleRequest changes, thus whenever its dependencies change.

Also, you are missing a

if (!isMounted.current) { return; }

on top, as we don't want to schedule a request when the component mounts, but when the initial request finished.

But your comment is not right, the reschedule will happen whenever scheduleRequest changes, thus whenever its dependencies change.

Could you explain this a bit? From the way I read this code, it looks like the effect is called whenever scheduleRequest (and therefore sendRequest) changes, but the isLoading condition prevents scheduleRequest from being called each time.

we don't want to schedule a request when the component mounts, but when the initial request finished.

isLoading is true when the component mounts, so the request isn't being initially scheduled. I'll add a comment to clarify this.

For context, I had to switch away from using totalRequests because here we're incrementing it before the request completes to get the request ID, instead of after like you originally suggested. This meant that if we used it to schedule the next request, then the next request would be scheduled before the first one completes.

isLoading is true when the component mounts

This is the problem I think. It should be false (as I was expecting it) and it is set to true when sendRequest() is called.

Could you explain this a bit?

Nevermind I will understand it. For me adding so much comment makes it harder to follow and it seems that there is something "special" the code, but that's ok.
I was saying that the ´useEffect´ will be triggered whenever any of the deps changes (which is simply how useEffect works). And of course, the if condition prevents the reschedule. But I don't need a comment to explain that.

My mental model was

const [isLoading, setIsLoading] = useState(false); // nothing happened just yet ... useEffect(() => { if (!isMounted.current) { return; // clearly indicate to not schedule anything on mount } if (!isLoading) { scheduleRequest(); } }, [isLoading, scheduleRequest]);

With the above code, I don't need any comment as it is self explanatory.

For context, I had to switch away from using totalRequests because here we're incrementing it before the request completes to get the request ID, instead of after like you originally suggested. This meant that if we used it to schedule the next request, then the next request would be scheduled before the first one completes.

I am not following but that's OK I guess 😄

cjcenizal · 2020-08-29T23:57:29Z

@sebelga I've converted all of sendRequest's internals to hooks, with a couple key differences from your original suggestion. I added some extra tests to define the behavior I think we want. Can you please take another look?

cjcenizal · 2020-08-31T02:11:39Z

@elasticmachine merge upstream

cjcenizal · 2020-08-31T03:10:59Z

src/plugins/es_ui_shared/public/request/use_request.ts

+    // Setting isLoading to false also acts as a signal for scheduling the next poll request.
+    setIsLoading(false);
+
+    return { data: serializedResponseData, error: responseError };


I think it's probably an antipattern that we're returning values from sendRequest. Seems like the consumer should be depending on the values returned by the hook instead. Once we merge this I'll create an issue to check for any consumers that depend on this return value and refactor them so we can try to remove this.

👍 This is something I noticed since the first iteration. But for some reason, we added the "// Gives the user the ability to manually request data" comment and exposed the handler.

I think we do want to retain sendRequest as part of the useRequest hook's return value, because we need it for "Reload" button behavior. But I don't think calling sendRequest should return a promise that resolves to data and error, since that information is already available off the useRequest hook's return value.

Ah ok I see, then maybe the API should be clearer and expose a refresh() handler that indeed does not return data nor error.

sebelga

Awesome work @cjcenizal ! We almost got it! 😊 I left some not about unnecessary comments that "restate trivial things". For the hook I left a comment about a missing mount check that should be added. I will approve but it'd be good to add it before merging. Thanks!

sebelga · 2020-08-31T06:21:47Z

src/plugins/es_ui_shared/public/request/use_request.ts

+    // Setting isLoading to false also acts as a signal for scheduling the next poll request.
+    setIsLoading(false);
+
+    return { data: serializedResponseData, error: responseError };


👍 This is something I noticed since the first iteration. But for some reason, we added the "// Gives the user the ability to manually request data" comment and exposed the handler.

sebelga · 2020-08-31T06:31:51Z

src/plugins/es_ui_shared/public/request/use_request.ts

+    clearPollInterval();
+
+    // Pull off request ID and increment request count.
+    const requestId = requestCountRef.current++;


nit: here it would be better to have

const requestId = ++requestCountRef.current;

and not have the requestCountRef.current - 1; below (the -1 part)

sebelga · 2020-08-31T06:32:15Z

src/plugins/es_ui_shared/public/request/use_request.ts

+    // If there's a scheduled poll request, this new one will supersede it.
+    clearPollInterval();
+
+    // Schedule next poll request.


nit: same here about unnecessary comment

sebelga · 2020-08-31T06:33:14Z

src/plugins/es_ui_shared/public/request/use_request.ts

+    }
+  }, [pollIntervalMs, sendRequest, clearPollInterval]);
+
+  // Send the request on component mount and whenever the dependencies of sendRequest() change.


nit: same here about the unnecessary comment (here you are basically describing the useEffect lifecycle.

I've removed the other comments you pointed out. I think this is the one comment I'm going to keep, just to really emphasize "the dependencies of sendRequest()" bit. It seems super obvious after staring at this code for hours, but I think the comment is more for someone who hasn't already spent a ton of time looking at the code. :)

sebelga · 2020-08-31T06:40:41Z

src/plugins/es_ui_shared/public/request/use_request.ts

+    if (!isLoading) {
+      scheduleRequest();
+    }
+  }, [isLoading, scheduleRequest]);


I think it is a little bit less explicit than with my totalRequest count (I personally understand better "when the request count increments, reschedule a request", as "isLoading" seems like it could change independently.) but I am OK to go with your suggestion.

But your comment is not right, the reschedule will happen whenever scheduleRequest changes, thus whenever its dependencies change.

Also, you are missing a

if (!isMounted.current) { return; }

on top, as we don't want to schedule a request when the component mounts, but when the initial request finished.

sebelga · 2020-08-31T06:42:06Z

src/plugins/es_ui_shared/public/request/use_request.ts

+    // data, to avoid doubled-up requests.
+    clearPollInterval();
+
+    // Pull off request ID and increment request count.


nit: I am not sure it is necessary to comment on obvious code behavior 😊 As per our guidelines "Don't use comments to restate trivial things." (https://github.com/elastic/kibana/blob/master/STYLEGUIDE.md#use-slashes-for-comments)

sebelga · 2020-08-31T06:45:07Z

.eslintrc.js

@@ -94,12 +94,6 @@ module.exports = {
        'jsx-a11y/no-onchange': 'off',
      },
    },
-    {


Great! Thanks for removing it here and adding the comments in the few places we still have the issue 👍

* Reconcile request helpers with eslint rules for React hooks. - Add clearer cleanup logic for unmounted components. - Align logic and comments in np_ready_request.ts and original request.ts. * Reorganize modules and convert tests to TS. - Split request.ts into send_request.ts and use_request.ts. - Convert test files into TS. - Relax SendRequestResponse type definition to type error as any instead of expecting an Error, since we weren't actually fulfilling this expectation. * Convert everything to hooks and add test coverage for request behavior. * Fix Watcher memoization bugs. # Conflicts: # .eslintrc.js

cjcenizal · 2020-09-01T03:12:20Z

@elastic/ingest-management @elastic/ml-ui Sorry for not pinging you before merging this! Could you please test out your apps which consume useRequest and verify that they still function correctly? I'd look extra carefully at your error-handling. If you specify the body, query, deserializer, or pollIntervalMs arguments, I'd also test that logic in particular.

kibanamachine · 2020-09-04T05:50:45Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 0e99888

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
esUiShared	229	+2	227

async chunks size

id	value	diff	baseline
watcher	602.5KB	+602.0B	602.0KB

page load bundle size

id	value	diff	baseline
esUiShared	991.0KB	+462.0B	990.6KB
indexManagement	248.1KB	-8.0B	248.1KB
total		+454.0B

History

💚 Build #71378 succeeded 0e99888
💔 Build #71242 failed b3414b7
💔 Build #71163 failed fc2f91e
💔 Build #71155 failed 40820aa
💔 Build #71148 failed 6ff6124

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

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

cjcenizal mentioned this pull request Jul 22, 2020

Fix es_ui_shared eslint violations for useRequest hook #59626

Closed

cjcenizal force-pushed the chore/use-request-eslint branch 6 times, most recently from 2897789 to 2554f46 Compare July 31, 2020 16:54

cjcenizal marked this pull request as ready for review July 31, 2020 16:54

cjcenizal requested a review from a team as a code owner July 31, 2020 16:54

cjcenizal requested a review from sebelga July 31, 2020 16:55

cjcenizal added 8 commits August 1, 2020 09:19

Remove index_management and snapshot_restore from eslintrc.

d996312

Reconcile request helpers with eslint rules for React hooks.

21fb1e6

- Add clearer cleanup logic for unmounted components. - Align logic and comments in np_ready_request.ts and original request.ts.

Replace original request.ts with np_ready_request.ts.

5e29bf4

Remove cleanup function which introduced a bug by setting isMounted t…

a813c3c

…o false during cleanups unrelated to unmounting.

Bring back isOutdatedRequest to gracefully handle changing poll inter…

248608e

…val.

Fix useRequest and sendRequest tests. Split into two test files.

7de3e5b

Reorganize modules and convert tests to TS.

82ba37c

- Split request.ts into send_request.ts and use_request.ts. - Convert test files into TS. - Relax SendRequestResponse type definition to type error as any instead of expecting an Error, since we weren't actually fulfilling this expectation.

Ignore eslint violation from isOutdatedRequest flag.

cb6a82d

cjcenizal force-pushed the chore/use-request-eslint branch from 2554f46 to 961b0b2 Compare August 1, 2020 16:19

Remove unnecessary useMemo.

2fe8e19

cjcenizal force-pushed the chore/use-request-eslint branch from 961b0b2 to 2fe8e19 Compare August 1, 2020 16:34

cjcenizal commented Aug 1, 2020

View reviewed changes

Merge branch 'master' into chore/use-request-eslint

ea6d8e4

cjcenizal added 4 commits August 4, 2020 15:41

Revert "Remove unnecessary useMemo."

ba508fe

This reverts commit 2fe8e19.

Add comment explaining purpose of stringifiedQuery.

6492468

Clarify purpose of deserializer return value test.

ff5f88a

Rename helpers files to test.helpers to clarify their role.

b83fc41

cjcenizal added 6 commits August 4, 2020 16:29

Remove outdated comments.

ded10ed

Convert useRequest to implement useCallback to define sendRequest bas…

bb429c3

…ed on its dependencies. - Update sendRequest to clear the poll interval immediately when it's called. - Update eslintrc to lint es_ui_shared. - Type arguments correctly as UseRequestConfig in helpers.

Merge branch 'master' into chore/use-request-eslint

c4ab04d

Merge branch 'master' into chore/use-request-eslint

646b5fa

Convert everything to hooks, fix bug tracking outdated responses, and…

a542b42

… add test coverage for request behavior.

Merge branch 'master' into chore/use-request-eslint

e5dc061

cjcenizal commented Aug 29, 2020

View reviewed changes

cjcenizal added 2 commits August 29, 2020 17:19

Remove unnecessary mention of isMounted from scheduling useEffect hook.

6ff6124

Skip lint rule for other modules.

40820aa

elasticmachine and others added 2 commits August 30, 2020 20:11

Merge branch 'master' into chore/use-request-eslint

1df8e52

Clarify comments.

fc2f91e

cjcenizal commented Aug 31, 2020

View reviewed changes

sebelga approved these changes Aug 31, 2020

View reviewed changes

cjcenizal added 2 commits August 31, 2020 06:07

Apply Seb's comments re comments.

b3414b7

Fix Watcher memoization bugs.

0e99888

cjcenizal merged commit e13d8dc into elastic:master Aug 31, 2020

cjcenizal mentioned this pull request Aug 31, 2020

[7.x] Fix es_ui_shared eslint violations for useRequest hook (#72947) #76328

Merged

cjcenizal mentioned this pull request Sep 4, 2020

Rename useRequest's sendRequest return function to resendRequest and remove return value #76795

Merged

nchaulet mentioned this pull request Sep 8, 2020

[Ingest Management]: Error 'Agent not found' is displayed for fraction of time on clicking Host under Fleet tab #76613

Closed

cjcenizal deleted the chore/use-request-eslint branch April 19, 2021 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix es_ui_shared eslint violations for useRequest hook #72947

Fix es_ui_shared eslint violations for useRequest hook #72947

cjcenizal commented Jul 22, 2020 •

edited

Loading

elasticmachine commented Jul 31, 2020

cjcenizal Aug 1, 2020 •

edited

Loading

cjcenizal commented Aug 3, 2020 •

edited

Loading

cjcenizal commented Aug 4, 2020

cjcenizal Aug 29, 2020 •

edited

Loading

sebelga Aug 31, 2020

cjcenizal Aug 31, 2020 •

edited

Loading

cjcenizal Aug 31, 2020 •

edited

Loading

sebelga Sep 1, 2020

cjcenizal commented Aug 29, 2020

cjcenizal commented Aug 31, 2020

cjcenizal Aug 31, 2020

sebelga Aug 31, 2020

cjcenizal Aug 31, 2020

sebelga Sep 1, 2020

sebelga left a comment

sebelga Aug 31, 2020

sebelga Aug 31, 2020

sebelga Aug 31, 2020

sebelga Aug 31, 2020

cjcenizal Aug 31, 2020

sebelga Aug 31, 2020

sebelga Aug 31, 2020

sebelga Aug 31, 2020

cjcenizal commented Sep 1, 2020

kibanamachine commented Sep 4, 2020

Fix es_ui_shared eslint violations for useRequest hook #72947

Fix es_ui_shared eslint violations for useRequest hook #72947

Conversation

cjcenizal commented Jul 22, 2020 • edited Loading

Errors

ES errors

405

400

caused_by chain

Non-ES errors

500

500

elasticmachine commented Jul 31, 2020

cjcenizal Aug 1, 2020 • edited Loading

Choose a reason for hiding this comment

cjcenizal commented Aug 3, 2020 • edited Loading

cjcenizal commented Aug 4, 2020

cjcenizal Aug 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Aug 31, 2020 • edited Loading

Choose a reason for hiding this comment

cjcenizal Aug 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Aug 29, 2020

cjcenizal commented Aug 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebelga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Sep 1, 2020

kibanamachine commented Sep 4, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

page load bundle size

History

cjcenizal commented Jul 22, 2020 •

edited

Loading

`caused_by` chain

cjcenizal Aug 1, 2020 •

edited

Loading

cjcenizal commented Aug 3, 2020 •

edited

Loading

cjcenizal Aug 29, 2020 •

edited

Loading

cjcenizal Aug 31, 2020 •

edited

Loading

cjcenizal Aug 31, 2020 •

edited

Loading