[es_ui_shared] Fix eslint exhaustive deps rule

[sebelga]

This PR fixes all the eslint disabled "react-hooks/exhaustive-deps" rule in the "es_ui_shared" plugin folder.

There is only one left, on purpose in the useRequest() hook as this one is under control 😊

Fixes #73970
Fixes #73971
Fixes #73972

[elasticmachine]

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

[cjcenizal]

Tested locally, code LGTM! Just had one question.

[cjcenizal]

I'm having trouble figuring out how we'd get into this state. Did you run into a situation where the component wasn't mounted when this function was called?

[sebelga]

Lol that's a great catch! 😊 I refactored a bit too quickly, I will put back how it was before. The intention was to not call the onUpdate on mount as... nothing has been updated yet.

[sebelga]

EDIT: Looking at the code it is correct. So like I said, the idea is not to call the onUpdate on component mount as there isn't any update.

In the next useEffect the component isMounted.current will be set to true so on the next update it will call the handler.

[cjcenizal]

Ah, I see. I think the use of the name isMounted is confusing to me, particularly because it defaults to false. The first time a hook is called is when the owner mounts, so by definition when I'm walking through a hook's "lifecycle" I begin with the mindset of it being mounted somewhere. To fix this, how about renaming the variable isInitialCall or isFirstUpdate, defaulting it to true and then setting it to false? I think this area of the code also deserves a comment explaining your intentions to the reader.

[sebelga]

particularly because it defaults to false

I must default to false. when the ref is created the component has not mounted yet. It only has mounted when a useEffect is called (every useEffect is called in order of appearance in the code. I learned that the hard way).

So the convention I have put in place everywhere is the same: isMounted (because that is the React term for this and I prefer not to add another concept) defaults to false and is set to true in the last useEffect in the code. So any previous useEffect will be called when mounting but the ref will still be false for them. This is exactly what we want: have a ref in our useEffects telling us if the component has mounted or not, and have some conditional logic to prevent doing certain things "on component mount". As you commented somewhere, useEffect should have a single purpose and not one giant useEffect (which would probably not work anyway).

This is what I shared in my "Lesson learned" here #75796

You will see around things like this

const didMount = useRef(false);

useEffect(() => {
  if (didMount.current) {
    // do some logic **after** the initial component mount
  } else {
    // the component has just mounted, nothing to do at that lifecycle
    didMount.current = true;
  }
}, [<deps that will influence the useEffect and get us ready for hours of debugging>]);

My convention is exactly this pattern but, as I have learned, it is much better to have a dedicated useEffect to handle this ref, and set it as last useEffect without any deps.

[cjcenizal]

👍 Thanks for explaining. I think some comments in the code would help explain this to others (or even our future selves).

[sebelga]

@elasticmachine merge upstream

[sebelga]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 8f2c42f

Build metrics

async chunks size

id	value	diff	baseline
indexManagement	1.6MB	+46.0B	1.6MB
ingestPipelines	705.8KB	+46.0B	705.7KB
total		+92.0B

page load bundle size

id	value	diff	baseline
esUiShared	994.2KB	+514.0B	993.7KB

History

• 💔 Build #72177 failed 438d9f2
• 💔 Build #72158 failed ba97afd
• 💔 Build #71849 failed 8727c9d
• 💔 Build #71795 failed c09a5be
• 💔 Build #71593 failed 6d152dd

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