fix(theme-common): properly memoize attributeFilter in useMutationObs…#11718
fix(theme-common): properly memoize attributeFilter in useMutationObs…#11718garry00107 wants to merge 1 commit intofacebook:mainfrom
Conversation
✅ [V2]Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
This solution starts to become too complex, we don't even use that code path, and it's not covered by any test. Please don't pick random code TODO and try to solve them, some of them are not always meant to be solved immediately, but later, once we have a use-case for improving our code. I'd prefer if you focused on actual bugs, and wrote clear tests to show that the problem is solved. Here the test plan doesn't test your code at all. Also, please disclose any AI usage. If you want to contribute here, please start gaining my trust by doing so manually. |
|
Hi @slorber , thank you for the feedback. I want to acknowledge that the test plan didn't actually test the new code path, which defeats the purpose. I used Claude to help identify and implement this change. |
Pre-flight checklist
Motivation
This PR addresses the TODO in
useMutationObserver.ts:The
useShallowMemoObjecthook only performs shallow comparison—it flattens object entries and compares values directly. This meansattributeFilter(an array) is compared by reference, not by value. If a caller passes a new array instance with the same contents on each render, the options will be considered "changed" and theMutationObserverwill be recreated unnecessarily.Solution
useShallowMemoArrayutility toreactUtils.tsxthat properly memoizes arrays by their element valuesuseMutationObserverto extract and memoizeattributeFilterseparately before combining it back with the rest of the optionsTest Plan
yarn test packages/docusaurus-theme-common