PaletteEditListView: add missing deps to useEffect dep array

[flootr]

What?

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

Why?

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

How?

Add the onChange prop & slugPrefix to the useCallback dependency array.

Testing Instructions

1. From your local Gutenberg directory, run npx eslint --rule 'react-hooks/exhaustive-deps: warn' packages/components/src/palette-edit
2. Confirm that the linter returns no errors
3. Confirm unit tests still pass

[flootr]

@chad1008 @ciampo May I ask you for a review on this one? It seems this particular component doesn't have a story to test it in Storybook though.

[ciampo]

Unfortunately no Storybook example was added for this component.

It looks like this component is used in the Global Styles sidebar, in the Site Editor — I took this PR for a spin and noticed that it introduces a regression when trying to add a new color to the palette:

Kapture.2022-09-10.at.18.50.38.mp4

It looks the changes from this PR make the cleanup function from useEffect run immediately after a color is added, which results in the color being immediately deleted.

We have a couple of choices:

• disable eslint for that specific line, adding a link to this PR in the comment
• try to change the logic of the component so that effectively the "cleanup" is run only when the user is done editing the palette (@jorgefilipecosta may help here?)

In the process, it would be also great if we could add unit tests to catch this regression in the future.

[chad1008]

It looks the changes from this PR make the cleanup function from useEffect run immediately after a color is added, which results in the color being immediately deleted.

It looks like this is happening because:

• onChange is being passed a setThemeColors method, which my brain tries to tell me should be stable, because it's a state updater. However:
• setThemeColors is itself returned from the useSetting hook, not from a useState. (I found that interesting @ciampo, because you and I saw something very similar to this in our pairing session earlier!)
• Within that hook, the setSetting method that's being returned (and destructured out into setThemeColors) isn't memoized. I think that's what's causing the problem.

To test this theory, I tried quickly wrapping setSetting in a useCallback. It helps, but not completely. When you first click on the button to add a new color the ColorPalette opens and closes, adding a basic black to the PaletteEditListView. From there, it's possible to use the PaletteEdit normally for additional colors:

Screen.Recording.2022-09-14.at.15.42.01.mov

Sorting out that first usage of the + button felt like it was outside the scope of this PR or the Components package in general, but hopefully this helps as a starting point for future investigations 🤞

[chad1008]

Circling back to this one... I'm thinking it might make the most sense to revert the changes related to exhaustive-deps and add an ignore comment that links back to this PR. That way we can move forward with activating the linter rule, and revisit this potential regression down the line. Thoughts @ciampo @flootr ? 🙂

[ciampo]

Sounds like a plan to me

[jorgefilipecosta]

• try to change the logic of the component so that effectively the "cleanup" is run only when the user is done editing the palette (@jorgefilipecosta may help here?)

I was not involved in coding this logic, so I don't have many insights. But It sounds like the logic was added there as a workaround that we want to run when unmounting, so adding the missing deps would break the logic. I don't think we should invest much time into this, so probably a lint ignore comment is the way to go here.

[chad1008]

I've opened a new PR in #45467 instead to disable the rule for this case. I figure we can come back to this original PR for a future refactor if we want... @flootr, I'll leave it to you to decide on keeping/closing as you see fit 🙂

[ciampo]

Hey @flootr , do you intend to pick this PR up or should we close it?

[flootr]

Hey @flootr , do you intend to pick this PR up or should we close it?

Let's close it for now. I'll add a note to follow up.