fix: replace unsafe useMemo with useState

[stipsan]

Description

From the React docs on useMemo:

React will not throw away the cached value unless there is a specific reason to do that. For example, in development, React throws away the cache when you edit the file of your component. Both in development and in production, React will throw away the cache if your component suspends during the initial mount. In the future, React may add more features that take advantage of throwing away the cache—for example, if React adds built-in support for virtualized lists in the future, it would make sense to throw away the cache for items that scroll out of the virtualized table viewport. This should be fine if you rely on useMemo solely as a performance optimization. Otherwise, a state variable or a ref may be more appropriate.

We do have some cases where the code is relying on useMemo never throwing away its value, and for the memo function itself to only be executed once, since the dependencies array is empty.
In this PR I've refactored all of them to useState, as per the recommendations.
By doing so we remove subtle bugs from sanity dev and hot module reloading, make tests less flaky, and allow React Compiler to better optimize the code since it treats useMemo as purely a performance optimization that it should be safe to change. While useState is handled differently.

For example, compare the before:

• If usage can't be traced, useMemo may be removed and it's called more frequently than before
• If usage is traced, it may behave exactly like before, but this isn't a guarantee based on wether the dependencies array in useMemo is empty or not, it's based on what's using it.

After, with useState it works exactly like before:

• If it can't trace usage, it's still memoized correctly and the init runs only once
• When usage is traced, it optimizes it correctly

What to review

Does this make sense?

Testing

Existing tests should be enough.

Notes for release

N/A – it's the type of fix where lots of tiny things feel slightly faster and more stable than before, but due to the nature of how useMemo behaves, and exactly how it can throw away cache if a component suspends during the initial mount, it's difficult to reproduce the issue and thus point to the fix.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
page-building-studio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 18, 2024 9:26am
performance-studio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 18, 2024 9:26am
test-next-studio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 18, 2024 9:26am
test-studio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 18, 2024 9:26am

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
studio-workshop	⬜️ Ignored (Inspect)	Visit Preview		Dec 18, 2024 9:26am

[github-actions]

No changes to documentation

[github-actions]

⚡️ Editor Performance Report

Updated Wed, 18 Dec 2024 09:23:50 GMT

Benchmark	reference latency of sanity@latest	experiment latency of this branch	Δ (%) latency difference
article (title)	26.0 efps (39ms)	26.3 efps (38ms)	-1ms (-1.3%)	✅
article (body)	67.6 efps (15ms)	78.1 efps (13ms)	-2ms (-/-%)	✅
article (string inside object)	25.6 efps (39ms)	25.6 efps (39ms)	+0ms (-/-%)	✅
article (string inside array)	23.3 efps (43ms)	24.4 efps (41ms)	-2ms (-4.7%)	✅
recipe (name)	50.0 efps (20ms)	51.3 efps (20ms)	-1ms (-2.5%)	✅
recipe (description)	58.8 efps (17ms)	58.8 efps (17ms)	+0ms (-/-%)	✅
recipe (instructions)	99.9+ efps (5ms)	99.9+ efps (5ms)	+0ms (-/-%)	✅
synthetic (title)	19.2 efps (52ms)	18.9 efps (53ms)	+1ms (+1.9%)	✅
synthetic (string inside object)	19.6 efps (51ms)	19.6 efps (51ms)	+0ms (-/-%)	✅

efps — editor "frames per second". The number of updates assumed to be possible within a second.

Derived from input latency. efps = 1000 / input_latency

Detailed information

🏠 Reference result

The performance result of sanity@latest

Benchmark	latency	p75	p90	p99	blocking time	test duration
article (title)	39ms	42ms	57ms	307ms	566ms	10.1s
article (body)	15ms	18ms	19ms	94ms	63ms	4.7s
article (string inside object)	39ms	42ms	60ms	203ms	138ms	7.0s
article (string inside array)	43ms	45ms	50ms	164ms	148ms	6.9s
recipe (name)	20ms	21ms	30ms	53ms	0ms	6.9s
recipe (description)	17ms	19ms	22ms	38ms	0ms	4.5s
recipe (instructions)	5ms	7ms	8ms	21ms	0ms	3.1s
synthetic (title)	52ms	54ms	59ms	116ms	129ms	12.9s
synthetic (string inside object)	51ms	53ms	72ms	563ms	1060ms	8.6s

🧪 Experiment result

The performance result of this branch

Benchmark	latency	p75	p90	p99	blocking time	test duration
article (title)	38ms	41ms	49ms	528ms	158ms	10.2s
article (body)	13ms	15ms	19ms	189ms	206ms	5.2s
article (string inside object)	39ms	40ms	44ms	57ms	133ms	6.6s
article (string inside array)	41ms	44ms	52ms	180ms	146ms	6.9s
recipe (name)	20ms	21ms	25ms	38ms	0ms	7.0s
recipe (description)	17ms	19ms	20ms	28ms	0ms	4.5s
recipe (instructions)	5ms	7ms	7ms	30ms	0ms	3.0s
synthetic (title)	53ms	56ms	63ms	145ms	192ms	13.5s
synthetic (string inside object)	51ms	54ms	61ms	447ms	806ms	8.6s

📚 Glossary

column definitions

• benchmark — the name of the test, e.g. "article", followed by the label of the field being measured, e.g. "(title)".
• latency — the time between when a key was pressed and when it was rendered. derived from a set of samples. the median (p50) is shown to show the most common latency.
• p75 — the 75th percentile of the input latency in the test run. 75% of the sampled inputs in this benchmark were processed faster than this value. this provides insight into the upper range of typical performance.
• p90 — the 90th percentile of the input latency in the test run. 90% of the sampled inputs were faster than this. this metric helps identify slower interactions that occurred less frequently during the benchmark.
• p99 — the 99th percentile of the input latency in the test run. only 1% of sampled inputs were slower than this. this represents the worst-case scenarios encountered during the benchmark, useful for identifying potential performance outliers.
• blocking time — the total time during which the main thread was blocked, preventing user input and UI updates. this metric helps identify performance bottlenecks that may cause the interface to feel unresponsive.
• test duration — how long the test run took to complete.

[github-actions]

Component Testing Report Updated Dec 18, 2024 9:22 AM (UTC)

✅ All Tests Passed -- expand for details

File	Status	Duration	Passed	Skipped	Failed
comments/CommentInput.spec.tsx	✅ Passed (Inspect)	1m 13s	15	0	0
formBuilder/ArrayInput.spec.tsx	✅ Passed (Inspect)	13s	3	0	0
formBuilder/inputs/PortableText/Annotations.spec.tsx	✅ Passed (Inspect)	40s	6	0	0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx	✅ Passed (Inspect)	55s	11	7	0
formBuilder/inputs/PortableText/copyPaste/CopyPasteFields.spec.tsx	✅ Passed (Inspect)	0s	0	12	0
formBuilder/inputs/PortableText/Decorators.spec.tsx	✅ Passed (Inspect)	28s	6	0	0
formBuilder/inputs/PortableText/DisableFocusAndUnset.spec.tsx	✅ Passed (Inspect)	15s	3	0	0
formBuilder/inputs/PortableText/DragAndDrop.spec.tsx	✅ Passed (Inspect)	30s	6	0	0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx	✅ Passed (Inspect)	1m 13s	15	0	0
formBuilder/inputs/PortableText/Input.spec.tsx	✅ Passed (Inspect)	2m 49s	21	0	0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx	✅ Passed (Inspect)	1m 51s	18	0	0
formBuilder/inputs/PortableText/PresenceCursors.spec.tsx	✅ Passed (Inspect)	14s	3	9	0
formBuilder/inputs/PortableText/Styles.spec.tsx	✅ Passed (Inspect)	29s	6	0	0
formBuilder/inputs/PortableText/Toolbar.spec.tsx	✅ Passed (Inspect)	1m 53s	21	0	0
formBuilder/tree-editing/TreeEditing.spec.tsx	✅ Passed (Inspect)	0s	0	3	0
formBuilder/tree-editing/TreeEditingNestedObjects.spec.tsx	✅ Passed (Inspect)	0s	0	3	0

[bjoerge]

Oh wow, this is fantastic @stipsan

I was honestly not aware of this slight gotcha with useMemo and have naturally to reached for useMemo in many of these cases.

I suppose mistakenly using useMemo for such cases is something that can easlily happen again in the future – is this something that could be detected by a linter?

[stipsan]

I suppose mistakenly using useMemo for such cases is something that can easlily happen again in the future – is this something that could be detected by a linter?

It's a bit tricky, because it all comes down to wether or not it's being used as a performance optimization, or for logic.
There's no need to ban useMemo(() => ..., []) outright, because sometimes it's what you want.

For example, in this case:

import {MyIcon} from './MyIcon'

export default function Component() {
  const icon = useMemo(() => <MyIcon />, [])

  return (
    <>
      {icon}
    </>
  )
}

In this case, if I edit MyIcon I want it to throw away the cache so that my edit shows up :)
A good rule of thumb, that I think is going to be hard for a linter to follow, is wether it's safe to remove the useMemo.
If you can remove it without logic breaking or perf degrading so much it becomes unusable, then it should be a useState instead. If removing it is fine, then it's fine to keep using it until React Compiler is stable and we can go back and delete all useMemo and useCallback usages and leave it to the compiler.
Note that even with the compiler in stable, there will be cases where you'll want to have a useState, a good example is ResourceCacheProvider in our codebase :D