Something went wrong while fetching comments. Please try again.
Created by: philipp-spiess
While working on https://github.com/sourcegraph/sourcegraph/pull/32209 I noticed that the updater function of the temporary settings hook is not stable across renders.
This means that if you want to use it inside an effect, that effect would re-render whenever the current value changes. That would happen on load (as the currentValue switch from undefined to what the backend returns) but also when the updater function is used to update the value.
To fix the issue, we create a reference that always points to the latest updated value.
I started by adding a regression test for the updater function pattern as well as a test that FAILS with the current implementation:
$ /Users/philipp/dev/sourcegraph/node_modules/.bin/jest useTemporarySetting
FAIL shared client/shared/src/settings/temporary/useTemporarySetting.test.tsx
useTemporarySetting
✓ should get correct data from storage (9 ms)
✓ should get undefined if data does not exist in storage (1 ms)
✓ should save data and update value (1 ms)
✓ should support the updater callback pattern (2 ms)
✓ should update other hook values if changed in another hook (1 ms)
✓ should update data if backend changed (1 ms)
✕ should not recreate the updater function (7 ms)
● useTemporarySetting › should not recreate the updater function
expect(received).toBe(expected) // Object.is equality
Expected: 1
Received: 2
183 | )
184 |
> 185 | expect(updateCount).toBe(1)
| ^
186 | })
187 | })
188 |
at Object.<anonymous> (src/settings/temporary/useTemporarySetting.test.tsx:185:29)
at TestScheduler.scheduleTests (../../node_modules/@jest/core/build/TestScheduler.js:333:13)
at runJest (../../node_modules/@jest/core/build/runJest.js:404:19)
at _run10000 (../../node_modules/@jest/core/build/cli/index.js:320:7)
at runCLI (../../node_modules/@jest/core/build/cli/index.js:173:3)
Test Suites: 1 failed, 1 total
Tests: 1 failed, 6 passed, 7 total
Snapshots: 0 total
Time: 1.05 s
Ran all test suites matching /useTemporarySetting/i.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.With the changes applied, all tests pass:
α sourcegraph (ps/fix-temporary-settings-hook-rerender)✗ yarn jest useTemporarySetting
yarn run v1.22.17
$ /Users/philipp/dev/sourcegraph/node_modules/.bin/jest useTemporarySetting
PASS shared client/shared/src/settings/temporary/useTemporarySetting.test.tsx
useTemporarySetting
✓ should get correct data from storage (10 ms)
✓ should get undefined if data does not exist in storage (2 ms)
✓ should save data and update value (2 ms)
✓ should support the updater callback pattern (2 ms)
✓ should update other hook values if changed in another hook (3 ms)
✓ should update data if backend changed (1 ms)
✓ should not recreate the updater function (7 ms)
Test Suites: 1 passed, 1 total
Tests: 7 passed, 7 total
Snapshots: 0 total
Time: 1.016 s
Ran all test suites matching /useTemporarySetting/i.
✨ Done in 2.05s.