Components: refactor TabPanel to pass exhaustive-deps

[chad1008]

What?

Updates the TabPanel component to satisfy the exhaustive-deps eslint rule

Why?

Part of the effort in #41166 to apply exhuastive-deps to the Components package

How?

Adds two missing dependencies (selected and initialTabName) to the component's useEffect dep array.

Adding initialTabName doesn't appear to cause any noticeable changes. Adding selected, on the other hand, does mean this effect now fires on every render, though it doesn't appear to cause any additional re-renders.

Previously, the effect only fired when the tabs props changed, and it was used to unsure the currently selected tab still existed. If not, it selects a fallback tab instead. With this change, the effect will also fire when a tab is selected and check that the selected tab exist (and it should, the user just clicked on it).

This isn't an expensive check, so I don't think we need to worry about it. If we want to prevent the effect from firing more than it used to, we could use the Latest Ref pattern to track the currently selected tab and use that ref's value in the effect. That would eliminate the dependency on selected and restore the effect's behavior to what it was before, but I don't honestly think it's worth the readability cost.

Testing Instructions

1. From your local Gutenberg directory, run npx eslint --rule 'react-hooks/exhaustive-deps: warn' packages/components/src/tab-panel
2. Confirm that the linter returns no errors
3. Confirm unit tests still pass
4. Run Storybook locally, confirm the components stories and/or docs still work as expected

[mirka]

This find() on L100 is already done outside of the useEffect on L96:

const newSelectedTab = find( tabs, { name: selected } );

Could we perhaps reuse that selectedTab variable instead of rerunning the find()?

Suggested change

	}, [ tabs, selected, initialTabName ] );
	}, [ tabs, selectedTab, initialTabName ] );

Like you say, it's not an expensive operation, but more hygienic and readable nonetheless 😄

[chad1008]

Good point - though I think we might want to memoize selectedTab before introducing it as a dependency on the useEffect.

The current dep selected is a string, so it won't cause the effect to fire if the value doesn't change. selectedTab is an object, so we'd end up firing the effect on each re-render. Updated in 465d9e2 what do you think?

[mirka]

Ah ok. Maybe we could pass selectedTab?.name instead of selectedTab to the useEffect's dep array then, so we don't need to memoize. Would that work?

[chad1008]

Yes, I think it will! Updated and rebased!

[mirka]

Careful on the updated logic here, apparently it was just merged a few days ago from #44028. I proposed some added tests in #45211.

[chad1008]

Yikes, I see what you mean. Thank you. Looking back at the commits, my previous change to memoize the value borked things up, and I didn't notice when we went to ?.name so things are stayed broken. I've pushed a fix, restoring the missing lines/checks that should address the problem.

Your new tests do, indeed, help highlight this kind of issue, and they're passing for me locally now... but I'll wait for #45211 to merge before finalizing this one.

[ciampo]

This PR will need a rebase once #45265 is merged to include fixes to the PHP unit tests

[mirka]

Looks good! 🚀