[Security Solution][Case] Improve hooks

[cnasikas]

Summary

We found a problem around our paradigm in case hooks. The problem was that useCallback does not know when a component is going unmounted, however useEffect does know all the secrets/ life cycle of a component therefore we are taking advantage of it.

Checklist

Delete any items that are not applicable to this PR.

• Unit or functional tests were updated or added to match the most common scenarios

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[elasticmachine]

Pinging @elastic/security-solution (Team: SecuritySolution)

[elasticmachine]

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

[ecezalp]

Hi! This is Ece, new hire on Threat Intelligence. This is my first review! I will keep it comment only until I get ramped up - I would sincerely appreciate a review of my review :)

Reading through the changes, I had a couple of thoughts

• naming of didCancel
  usually booleans are prefixed with is, so if didCancel remained a boolean, I would have suggested a name such as isCancelled. However it looks like didCancel became a ref. In React docs refs are usually indicated with a Ref suffix (myRef, inputRef in Refs and the DOM) this is by no means necessary, but I personally like having that visual indicator as a reminder to always read .current of something that is clearly a fooRef. Therefore, I think that didCancel could also be called didCancelRef or even isCancelledRef - whichever works best.
• reoccurence and similar usage of didCancel and abortCtrl refs
  it looks like we could create a new hook that encapsulates the declaration of didCancel and abortCtrl refs, as well as the cleanup func, which could be used in multiple hooks that are being tidied up in this PR. In fact, [isLoading, setIsLoading] could also be a part of the new hook, which could be called something along the lines of useAbortableRequest.
• error.name !== 'AbortError' check
  it looks like we always check !didCancel.current beforehand, so if the request is aborted, we won't be executing this block anyway, so the check seems useless unless there is any other scenario in which a request can get aborted outside of the cleanup with the AbortCtrl call.
• cancelToken
  I am not particularly familiar with this codebase - with a quick look, it looks like the AbortController is creating an event called "abort", and 'things' get set in motion from thereon. I would have assumed that a cancelToken would be generated when the request is being made, and that cancelToken could be called to cancel the request if the user navigated away from the page, instead of an AbortController call to take care if this - I would be very happy to learn more about this pattern. I am assuming that the abort event is being caught by a middleware, will follow up with this to broaden my understanding.

[cnasikas]

Hey! Welcome to the team Ece! Feel free to leave comments on the code.

naming of didCancel

Ok!

reoccurence and similar usage of didCancel and abortCtrl refs

I think this is a scenario where duplication is better than deduplication. The reason is that useEffect's can sometimes can be unpredicted or easy to introduced bugs if not used correctly. I think, in this type of hooks, is better to have isolated hooks than hooks that share logic from other hooks. Imagine we have to fix a bug on a hook. It is easier to do the fix and debug in one hook rather than to have to do the fix in a generic hooks that all other hooks will be affected. It will be difficult to test and to be sure that the fix will not create other bugs to other hooks. I think isolation is better and it can justify the duplication.

error.name !== 'AbortError'

I had the same thought at first 😄! Imagine a user clicks a button to submit and then press the button again very fast before the first request has been fulfilled. In this scenario the first call will be canceled, as we abortController.current.abort(), and an AbortError is throw. didCancel.current is false so it will show the toaster. With error.name !== 'AbortError' we make sure that will not happen. Another scenario that could occurred (although, I am not 100% sure and maybe I am wrong) is when a component re-renders very fast and a request is made in the first render but in the second re-render the first request is being canceled and a second one initiates.

cancelToken

AbortController is part of the Web API and it can be used to abort request at will. fetch is implemented in a way that if a signal (AbortSignal) is provided it listen to this event and aborts when the event is fired (abortController.abort()).

[kqualters-elastic]

Agree that these hooks and many others in the plugin seem like there is ample opportunity for a shared hook or two to remove some of the identical code, but this fixes the loading bug and bug with no cleanup logic running when the consuming component is destroyed so lgtm 👍

[cnasikas]

@elasticmachine merge upstream

[kibanamachine]

merge conflict between base and head

[cnasikas]

@elasticmachine merge upstream

[cnasikas]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: f8c4468
• Storybooks Preview

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
securitySolution	7.7MB	7.8MB	+4.7KB

History

• 💚 Build #109136 succeeded 41b25358cf017888c95df7c0e00d10819fd47cb9
• 💔 Build #109091 failed e765efb08351e3b9591da9d11ac644938264a6b5
• 💔 Build #108999 failed eab13e74b537f54e20cdd6b126e690c8484ee7ff
• 💛 Build #106506 was flaky e84d47d1f494bb1f053aac3643ab2b43e3bedbcb
• 💚 Build #106483 succeeded 4155a16bb4483dd56d17b065f0710dc74410d763

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

cc @cnasikas