Fix es_ui_shared eslint violations for useRequest hook

[cjcenizal]

Partially addresses #49554
Closes #49572 (originally fixed as part of the NP migration)
Closes #49562 (originally fixed as part of the NP migration)

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)

To test:

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

[elasticmachine]

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

[walterra]

Code LGTM, also tested a checkout, thanks for the update!

[cjcenizal]

I added this to handle the case when the owner component unmounts before a request has resolved.

[cjcenizal]

@sebelga Could you please test Index Management, Watcher, and Snapshot Restore and see if you spot any bugs regarding requests?

[cjcenizal]

@elastic/ingest-management Could someone from your team please test the Ingest Manager and make sure I haven't introduced any buggy behavior on your end?

[jen-huang]

Ran a smoke test of Ingest Manager locally and things are working as expected 👍

[sebelga]

LGTM! Tested locally watcher, IM, and S & R and did not spot any regression.

[sebelga]

Nit: isn't it the same as

pollIntervalIdRef.current = setTimeout(
  sendRequestRef.current!,
  pollIntervalMsRef.current
);

[sebelga]

Can you explain why you created a ref here? I would have used useCallback for this purpose. Was it necessary to add the ref?

I see that sendRequestRef is also a ref. I think useCallback is better for this purpose. Maybe I'm missing something?

[cjcenizal]

I think switching to useCallback is an interesting idea and could yield optimization benefits by returning a memoized reference to sendRequest. It would look something like this, though I'm not sure about the implications of mutating a ref you're dependent upon (any risk of infinite loop?):

  const scheduleRequest = useCallback(
    () => {
      // Clear current interval
      if (pollIntervalIdRef.current) {
        clearTimeout(pollIntervalIdRef.current);
      }

      // Set new interval
      if (pollIntervalMsRef.current && isMounted.current) {
        pollIntervalIdRef.current = setTimeout(
          () => sendRequestRef.current!(),
          pollIntervalMsRef.current
        );
      }
    },
    [pollIntervalIdRef.current, pollIntervalMsRef.current, isMounted.current, pollIntervalIdRef.current, sendRequestRef.current],
  );

However, the original NP-ready code uses refs and I find them easier to reason about than the useCallback example above, so I think I'll leave optimization to another day.

[sebelga]

Yeah, it would imply a bigger refactor (e.g. why do we have a ref for pollInterval?), but I agree we can leave that to another day. 👍

[cjcenizal]

I just noticed a subtle change 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 the plugins which depend upon the old behavior (Snapshot Restore and Ingest Manager).

@jen-huang Can you or someone from the Ingest team please audit Ingest Manager and make sure errors returned by sendRequest are being consumed correctly?

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 480c83d

History

• 💚 Build #43210 succeeded 616d4a6
• 💔 Build #40074 failed 2a526250d95c533393aaad26fd3308c5949d0608

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

[cjcenizal]

This branch is too stale to continue on. Closing in favor of #72947