mirror of
https://github.com/vector-im/element-call.git
synced 2026-09-10 21:55:19 +00:00
Compare the component's config by value
Everything downstream of the component's params — the mute state, the
call view model and with it the media connection — is keyed on the
identity of the params object, which was memoised on the identity of
the `config` prop. A host writing `config={{ ... }}` inline, which is
the natural way to write it, therefore tore the whole call down on
every render. The harness happened to pass a constant, so nothing
noticed.
`useStableValue` hands out the same object for as long as a deep
comparison says nothing changed, so an inline config costs nothing.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5.1
parent
489c1af426
commit
a010cc983f
+12
-2
@@ -66,6 +66,7 @@ import { type ConfigOptions } from "../src/config/ConfigOptions";
|
|||||||
import { i18n } from "../src/utils/i18n";
|
import { i18n } from "../src/utils/i18n";
|
||||||
import { useTheme } from "../src/useTheme";
|
import { useTheme } from "../src/useTheme";
|
||||||
import { useInitial } from "../src/useInitial";
|
import { useInitial } from "../src/useInitial";
|
||||||
|
import { useStableValue } from "../src/useStableValue";
|
||||||
import styles from "./ElementCall.module.css";
|
import styles from "./ElementCall.module.css";
|
||||||
|
|
||||||
// Everything needed to implement a HostBridge, not just the interface itself
|
// Everything needed to implement a HostBridge, not just the interface itself
|
||||||
@@ -115,6 +116,9 @@ export interface ElementCallProps {
|
|||||||
* How Element Call should behave, overriding whatever {@link intent} implies.
|
* How Element Call should behave, overriding whatever {@link intent} implies.
|
||||||
* A host that finds itself setting a lot of these probably wants a different
|
* A host that finds itself setting a lot of these probably wants a different
|
||||||
* intent instead.
|
* intent instead.
|
||||||
|
*
|
||||||
|
* Compared by value, so it is fine to write this inline; only a change to
|
||||||
|
* what it says restarts anything.
|
||||||
*/
|
*/
|
||||||
config?: ElementCallConfiguration;
|
config?: ElementCallConfiguration;
|
||||||
/**
|
/**
|
||||||
@@ -180,14 +184,20 @@ export const ElementCall: FC<ElementCallProps> = ({
|
|||||||
// Element Call has no URL of its own to read any of this from, and the
|
// Element Call has no URL of its own to read any of this from, and the
|
||||||
// host's URL is not Element Call's business, so the defaults come from the
|
// host's URL is not Element Call's business, so the defaults come from the
|
||||||
// intent with the host's wishes over the top.
|
// intent with the host's wishes over the top.
|
||||||
|
//
|
||||||
|
// Everything downstream — the mute state, the call view model and with it
|
||||||
|
// the media connection — is keyed on the identity of this object, so it has
|
||||||
|
// to be stable for as long as its contents are. A host writing `config`
|
||||||
|
// inline would otherwise tear the call down on every render.
|
||||||
|
const stableConfig = useStableValue(config);
|
||||||
const params = useMemo(
|
const params = useMemo(
|
||||||
(): UrlParams => ({
|
(): UrlParams => ({
|
||||||
...hostedProperties,
|
...hostedProperties,
|
||||||
roomId,
|
roomId,
|
||||||
...configurationForIntent(intent),
|
...configurationForIntent(intent),
|
||||||
...config,
|
...stableConfig,
|
||||||
}),
|
}),
|
||||||
[roomId, intent, config],
|
[roomId, intent, stableConfig],
|
||||||
);
|
);
|
||||||
|
|
||||||
const mediaDevices = useInitial(
|
const mediaDevices = useInitial(
|
||||||
|
|||||||
@@ -0,0 +1,64 @@
|
|||||||
|
/*
|
||||||
|
Copyright 2026 Element Creations Ltd.
|
||||||
|
|
||||||
|
SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial
|
||||||
|
Please see LICENSE in the repository root for full details.
|
||||||
|
*/
|
||||||
|
|
||||||
|
import { renderHook } from "@testing-library/react";
|
||||||
|
import { describe, expect, test } from "vitest";
|
||||||
|
|
||||||
|
import { useStableValue } from "./useStableValue";
|
||||||
|
|
||||||
|
describe("useStableValue", () => {
|
||||||
|
test("keeps the first identity while the contents stay equal", () => {
|
||||||
|
const first = { skipLobby: true, fonts: ["Inter"] };
|
||||||
|
const { result, rerender } = renderHook(
|
||||||
|
({ value }) => useStableValue(value),
|
||||||
|
{ initialProps: { value: first } },
|
||||||
|
);
|
||||||
|
expect(result.current).toBe(first);
|
||||||
|
|
||||||
|
rerender({ value: { skipLobby: true, fonts: ["Inter"] } });
|
||||||
|
expect(result.current).toBe(first);
|
||||||
|
});
|
||||||
|
|
||||||
|
test("takes the new identity once the contents change", () => {
|
||||||
|
const first = { skipLobby: true };
|
||||||
|
const second = { skipLobby: false };
|
||||||
|
const { result, rerender } = renderHook(
|
||||||
|
({ value }) => useStableValue(value),
|
||||||
|
{ initialProps: { value: first } },
|
||||||
|
);
|
||||||
|
|
||||||
|
rerender({ value: second });
|
||||||
|
expect(result.current).toBe(second);
|
||||||
|
|
||||||
|
// And that identity is then the stable one
|
||||||
|
rerender({ value: { skipLobby: false } });
|
||||||
|
expect(result.current).toBe(second);
|
||||||
|
});
|
||||||
|
|
||||||
|
test("handles undefined, for an optional prop left out", () => {
|
||||||
|
const { result, rerender } = renderHook(
|
||||||
|
({ value }) => useStableValue(value),
|
||||||
|
{ initialProps: { value: undefined as { a: number } | undefined } },
|
||||||
|
);
|
||||||
|
expect(result.current).toBeUndefined();
|
||||||
|
|
||||||
|
const given = { a: 1 };
|
||||||
|
rerender({ value: given });
|
||||||
|
expect(result.current).toBe(given);
|
||||||
|
});
|
||||||
|
|
||||||
|
test("accepts its own notion of equality", () => {
|
||||||
|
const first = { id: 1, label: "a" };
|
||||||
|
const { result, rerender } = renderHook(
|
||||||
|
({ value }) => useStableValue(value, (a, b) => a.id === b.id),
|
||||||
|
{ initialProps: { value: first } },
|
||||||
|
);
|
||||||
|
|
||||||
|
rerender({ value: { id: 1, label: "b" } });
|
||||||
|
expect(result.current).toBe(first);
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -0,0 +1,29 @@
|
|||||||
|
/*
|
||||||
|
Copyright 2026 Element Creations Ltd.
|
||||||
|
|
||||||
|
SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial
|
||||||
|
Please see LICENSE in the repository root for full details.
|
||||||
|
*/
|
||||||
|
|
||||||
|
import { useState } from "react";
|
||||||
|
import { isEqual } from "lodash-es";
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Returns a value whose identity only changes when its contents do.
|
||||||
|
*
|
||||||
|
* For a prop that a caller is likely to write inline — an options object, say
|
||||||
|
* — so that a fresh but equal object on every render does not restart whatever
|
||||||
|
* depends on it. Deep equality by default.
|
||||||
|
*/
|
||||||
|
export function useStableValue<T>(
|
||||||
|
value: T,
|
||||||
|
equals: (a: T, b: T) => boolean = isEqual,
|
||||||
|
): T {
|
||||||
|
const [stable, setStable] = useState(value);
|
||||||
|
if (equals(stable, value)) return stable;
|
||||||
|
// Setting state during render makes React re-run this render immediately
|
||||||
|
// with the new state, at which point the two are identical and the stored
|
||||||
|
// one is returned — so the identity handed out is consistent.
|
||||||
|
setStable(value);
|
||||||
|
return value;
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user