[FIX] Fix infinite loop in federation dashboard

[hugopeixoto]

Proposed changes

The infinite loop was caused by the improper comparison of a useEffect
dependency.

useEffect uses Object.is(previous, current) do determine if it should
rerun the effect or not. Since we're using args, which in the federation
calls is an empty array literal, it will always return false.

Initially this was passed as ...args, which didn't cause any problems.
It was changed in #18438 (in a sub-pull request, in this commit 05ca878), an unrelated Omnichannel admin dashboard
rewrite that also uses useMethodData.

Changing it to JSON.stringify(args) ensures that we're comparing strings
instead of arbitrary objects, solving the problem. This pattern seems to
be used in other useEffects as well throughout the codebase.

Issue(s)

#19007

Types of changes

• Bugfix (non-breaking change which fixes an issue)
• Improvement (non-breaking change which improves a current function)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Hotfix (a major bugfix that has to be merged asap)
• Documentation Update (if none of the other choices apply)

Checklist

• I have read the CONTRIBUTING doc
• I have signed the CLA
• Lint and unit tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works (if applicable)
• I have added necessary documentation (if applicable)
• Any dependent changes have been merged and published in downstream modules

Changelog

[hugopeixoto]

Ugh, I fixed this in 3.6.3, but it seems that develop already had a fix for this issue, merged in #19202. The solution was to use useMemo([]) instead of [] as an argument to useMethodPolledData.

I'm not sure if using useMemo instead of JSON.stringify is better. The useMemo solution doesn't prevent callers of useMethodPolledData from falling into the same pitfall, while using JSON.stringify prevents this bug from occurring again. It seems that using JSON.stringify is acceptable, since there are other occurrences of it in useEffect dependencies.

I'll rebase this again and remove useMemo, as we don't need both JSON.stringify and useMemo.

Thoughts?

/cc @ggazzo , as they were the author of #19202.

[tassoevan]

Unfortunately, eslint-plugin-react-hooks is ineffective when the dependency array of a hook has complex expressions: it expects only variable references.

A better workaround might be using this custom hook to memoize args internallly.

[hugopeixoto]

Thanks for the review.

Unfortunately, eslint-plugin-react-hooks is ineffective when the dependency array of a hook has complex expressions: it expects only variable references.

Hm, I see the advantages of not having a complex expression in the dependency list.

A better workaround might be using this custom hook to memoize args internallly.

Using useStableArray solves the identity problem at the first level (the array), but the array elements are still compared using Object.is, so we might end up in a similar situation if there are any complex args.

useStableArray takes a compare argument that allows us to override Object.is. I could pass (a,b) => JSON.stringify(a) == JSON.stringify(b) as the second argument, and wrap this in a useStableArgs hook.

This may be me walking in circles. Maybe it would make more sense to build useStableArgs by calling useMemo with JSON.stringify as its dependency and an eslint exception. This would, at least, isolate the exception in a separate file.

What do you think? Is thinking of complex args worth it? Is there a simpler solution?

[CLAassistant]

All committers have signed the CLA.

[hugopeixoto]

@tassoevan do you have any thoughts on my last comment? Should I close this PR? maybe it has been fixed in the meantime.