Skip to content

Fix unnecessary recreation of temporary settings hook updater callback

Warren Gifford requested to merge ps/fix-temporary-settings-hook-rerender into main

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.

Test plan

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.

Merge request reports