Skip to content

Commit e53949e

Browse files
authored
refactor: react contract tests (#1226)
This PR will: - move some variable around for the react SDK so it is a bit more readable in my opinion ## Context This PR is the result of some exploration on how to make the react contract more idiomatic to react. A few things that I looked into: - changing evaluation to `useVariation` flags - changing some of the internal commands to use react hooks - removing some of the `useRef` Ultimately, most of the exploration did not yield a good result because they require very verbose changes. A lot of the code consolidation, I think, should be consolidated into the shared contract test tooling package. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Moves client/handler bookkeeping from React state/refs to module-level globals with manual re-rendering, which could introduce lifecycle/state-leak issues across mounts or tests. Changes are limited to contract-test harness code, but affect client creation/deletion and command routing timing. > > **Overview** > Refactors the React contract-test harness to manage clients and command handlers via module-level `Map`s/arrays instead of `useRef`/component state, triggering UI updates via a simple render tick. > > Client creation/deletion now updates global `clientRecords` and uses a shared `onReady` resolver map to coordinate when `ClientInstance` has registered its command handler before returning the client id. Also removes a stale comment in `ClientEntity.ts`. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 14cef1f. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/launchdarkly/js-core/pull/1226" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a> <!-- devin-review-badge-end -->
1 parent fe8bd37 commit e53949e

2 files changed

Lines changed: 24 additions & 29 deletions

File tree

packages/sdk/react/contract-tests/app/ClientEntity.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,8 +80,6 @@ export function makeSdkConfig(options: SDKConfigParams, tag: string): LDOptions
8080
return cf;
8181
}
8282

83-
// NOTE: we can make this more idiomatic to React by creating a useCommand hook and reading return values from
84-
// a rendered component. But for now, this is just a simple way to get the tests running.
8583
export async function doCommand(client: LDReactClient, params: CommandParams): Promise<unknown> {
8684
const logger = makeLogger('doCommand');
8785
logger.info(`Received command: ${params.command}`);

packages/sdk/react/contract-tests/app/ClientRoot.tsx

Lines changed: 24 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
'use client';
22

3-
import React, { useCallback, useEffect, useRef, useState } from 'react';
3+
import React, { useEffect, useState } from 'react';
44

55
import { CreateInstanceParams } from '@launchdarkly/js-contract-test-utils/client';
66
import {
@@ -18,24 +18,28 @@ interface ClientRecord {
1818
Provider: React.FC<{ children: React.ReactNode }>;
1919
}
2020

21+
const commandHandlers = new Map<string, CommandHandler>();
22+
const handlerReadyMap = new Map<string, () => void>();
23+
let clientRecords: ClientRecord[] = [];
24+
let clientCounter = 0;
25+
26+
function onReady(readyId: string) {
27+
handlerReadyMap.get(readyId)?.();
28+
}
29+
2130
export default function ClientRoot({ children }: { children: React.ReactNode }) {
22-
// Keeps a list of all the clients that we have, we will need to keep this as a state
23-
// to ensure that the ld client providers are being rendered.
24-
const [clients, setClients] = useState<ClientRecord[]>([]);
25-
const commandHandlers = useRef(new Map<string, CommandHandler>());
26-
const handlerReadyMap = useRef(new Map<string, () => void>());
27-
const clientCounterRef = useRef(0);
28-
const clientsRef = useRef<ClientRecord[]>([]);
31+
const [, setRenderTick] = useState(0);
32+
const rerender = () => setRenderTick((n) => n + 1);
2933

3034
useEffect(() => {
3135
const ws = new TestHarnessWebSocket(
3236
'ws://localhost:8001',
33-
commandHandlers.current,
37+
commandHandlers,
3438

3539
// On create client
3640
async (params: CreateInstanceParams) => {
37-
const id = String(clientCounterRef.current);
38-
clientCounterRef.current += 1;
41+
const id = String(clientCounter);
42+
clientCounter += 1;
3943

4044
const timeout =
4145
params.configuration.startWaitTimeMs !== null &&
@@ -63,45 +67,38 @@ export default function ClientRoot({ children }: { children: React.ReactNode })
6367
throw new Error('client initialization failed');
6468
}
6569

66-
// Currently these tests are creating the provider with a custom ld client, which is a
67-
// supported feature, but I think it would be better if we can use the create provider
68-
// factory function instead.
6970
const Provider = createLDReactProviderWithClient(client);
7071

7172
const handlerReady = new Promise<void>((resolve) => {
72-
handlerReadyMap.current.set(id, resolve);
73+
handlerReadyMap.set(id, resolve);
7374
});
7475

75-
clientsRef.current = [...clientsRef.current, { id, client, Provider }];
76-
setClients((prev) => [...prev, { id, client, Provider }]);
76+
clientRecords = [...clientRecords, { id, client, Provider }];
77+
rerender();
7778

7879
await handlerReady;
79-
handlerReadyMap.current.delete(id);
80+
handlerReadyMap.delete(id);
8081
return id;
8182
},
8283

8384
// On delete client
8485
(id: string) => {
85-
clientsRef.current.find((r) => r.id === id)?.client.close();
86-
clientsRef.current = clientsRef.current.filter((r) => r.id !== id);
87-
setClients((prev) => prev.filter((r) => r.id !== id));
86+
clientRecords.find((r) => r.id === id)?.client.close();
87+
clientRecords = clientRecords.filter((r) => r.id !== id);
88+
rerender();
8889
},
8990
);
9091

9192
ws.connect();
9293
return () => ws.disconnect();
9394
}, []);
9495

95-
const onReady = useCallback((readyId: string) => {
96-
handlerReadyMap.current.get(readyId)?.();
97-
}, []);
98-
9996
return (
10097
<>
10198
{children}
102-
{clients.map(({ id, Provider }) => (
99+
{clientRecords.map(({ id, Provider }) => (
103100
<Provider key={id}>
104-
<ClientInstance clientId={id} handlers={commandHandlers.current} onReady={onReady} />
101+
<ClientInstance clientId={id} handlers={commandHandlers} onReady={onReady} />
105102
</Provider>
106103
))}
107104
</>

0 commit comments

Comments
 (0)