Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PaletteEditListView: add missing deps to useEffect dep array #43911

Conversation

flootr
Copy link
Contributor

@flootr flootr commented Sep 6, 2022

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 flootr changed the title PaletteEditListView: add missing deps to useEffect dep array PaletteEditListView: add missing deps to useEffect dep array Sep 6, 2022
@chad1008 chad1008 added [Feature] UI Components Impacts or related to the UI component system [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Sep 6, 2022
@flootr flootr force-pushed the enhance/PaletteEditListView-exhaustive-deps branch from 5ba042e to b9e161b Compare September 9, 2022 07:45
@flootr flootr marked this pull request as ready for review September 9, 2022 07:45
@flootr
Copy link
Contributor Author

flootr commented Sep 9, 2022

@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
Copy link
Contributor

ciampo commented Sep 10, 2022

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

ciampo commented Oct 25, 2022

Sounds like a plan to me

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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
Copy link
Contributor

chad1008 commented Nov 1, 2022

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
Copy link
Contributor

ciampo commented Nov 26, 2022

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

@flootr
Copy link
Contributor Author

flootr commented Jan 9, 2023

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.

@flootr flootr closed this Jan 9, 2023
@flootr flootr deleted the enhance/PaletteEditListView-exhaustive-deps branch January 9, 2023 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants