Add cx as a dependency of useMemo across @wordpress/components package
#38541
Conversation
ciampo
force-pushed
the
fix/emotion-use-cx-use-memo
branch
from
6a98cf4 to
e82ad27
Compare
|
Size Change: +32 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
ciampo
added
[Feature] Component System
There was a problem hiding this comment.
❤️ that you added Storybook repros!
Agreed on all the next steps you listed. I'm especially concerned about the exhaustive-deps linter not being in place. There seems to have been some history around it (#24914 (comment)). I checked that rule against the current codebase, and there are already a lot of instances that are either bugs, or at the very least, unclear whether they are intentional (which I'd say is equally as troubling). I am a strong 👍 for adding this warn ASAP.
| const ExampleWithUseMemoWrong = ( { args, children } ) => { | ||
| const cx = useCx(); | ||
| // Wrong: using 'useMemo' without adding 'cx' to the dependency list. | ||
| const classes = useMemo( () => cx( ...args ), [ ...args ] ); |
There was a problem hiding this comment.
Assuming that we will be adding the exhaustive-deps rule soon (🤞🤞🤞), would it be worth adding a preemptive eslint-disable-next-line here?
Also noting that the linter will throw up this warning if we use spreads (...args):
React Hook useMemo has a spread element in its dependency array. This means we can't statically verify whether you've passed the correct dependencies. react-hooks/exhaustive-deps
There was a problem hiding this comment.
Assuming that we will be adding the exhaustive-deps rule soon (🤞🤞🤞), would it be worth adding a preemptive
eslint-disable-next-linehere?
Makes sense, I've added it!
Also noting that the linter will throw up this warning if we use spreads (
...args):
Good point. I rewrote the Storybook examples to avoid using the args array prop, and replace it with a serializedStyles prop
I think it would be applied to the first element that gets rendered (i.e., the first one that manages to call I also wanted to mention that this will only effect instances of Emotion components using
I also wanted to throw in that the root cause of this is that the Also wanted to point out that It would still be worth it to evaluate what difference Looking at the instances in this PR where we've had to add
Scratch that! I have no idea why that was the behavior we were seeing in Gutenberg unless it's React version dependent. I am not able to reproduce that behavior in codesandbox: https://codesandbox.io/s/broken-frost-643dz?file=/src/App.js |
ciampo
force-pushed
the
fix/emotion-use-cx-use-memo
branch
from
1a91f41 to
7366a53
Compare
|
nice work catching that 👍 |
|
Thank you @sarayourfriend and @mirka for you reviews!
I've added a comment to that PR asking for its status.
This is also something that I hadn't realised so far, thank you for pointing it out!
Absolutely! Do you know if we conducted a similar test in the past? In that case we could replicate the same setup
Do you think that's something worth looking into? |
|
@ciampo yup! @ItsJonQ did a thorough performance evaluation here: ItsJonQ/g2#57 I think everything in that issue will still apply even though we're not using g2... most of the time g2 just passed straight through to Emotion anyway.
Probably worth spending some time try to reproduce it independent of Gutenberg... just so that we actually understand the why. We might be able to prevent similar problems in the future. |
Description
Explanation of the issue
While investigating the issue flagged in #35619 (comment) in #38447 , @sarayourfriend and I managed to reproduce the issue in a Storybook example:
iframe, but styles are apparently only applied correctly to the outside elementuseCxhook to compute the classname, and would have that computation wrapped inside React'suseMemohook. But thecxfunction would not be part of theuseMemodependenciesThe combination of the 2 points above causes the component's classnames to be computed only once for the component outside of the
iframe. The component inside theiframegets the same classnames (because of the memoization), but those classnames can't be found inside theiframe(since the styles are added to theiframevia a differentStyleProvider).Long story short: when rendered inside the
iframe, the component'scxfunction updates to reference the correct Emotion cache, but the component doesn't get the correct classnames because, when thecxfunction updates, the memoized classnames remains the same.What's included in the PR:
This PR, therefore:
cxanduseMemoin the@wordpress/componentspackage, addingcxto the list of dependenciesWhat should we do next
As also discussed with @sarayourfriend , we should assess if the usage of
useMemowhen computing Emotion's classnames is actually necessary to optimise the rendering performance of components.We also wondered why
eslintdidn't flag the missingcxdependency — it looks like we're not enforcing theexhaustive-depsrule, I wonder what's the reason for it? (cc @gziolo )Finally, in case we have good reasons not to enforce the
exhaustive-depsrule, and if we decided to keep memoizing Emotion classname's computation, we could consider adding our owneslintrule aimed specifically at making sure thatcxis always added as a dependency ofuseMemoTesting Instructions
cxto the list of dependencies ofuseMemoin theExampleWithUseMemoWrongcomponent, the color's becomes bluecxhas been added as a dependency ofuseMemoin all of the instances where the 2 are used together in the@wordpress/componentspackageScreenshots
Types of changes
Fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).