diff --git a/packages/browser-sdk/src/client.ts b/packages/browser-sdk/src/client.ts index 84cd6667..877a8276 100644 --- a/packages/browser-sdk/src/client.ts +++ b/packages/browser-sdk/src/client.ts @@ -606,24 +606,20 @@ export class ReflagClient { * @param user */ async updateUser(user: { [key: string]: string | number | undefined }) { - const userIdChanged = user.id && user.id !== this.context.user?.id; const newUserContext = { ...this.context.user, ...user, id: user.id ?? this.context.user?.id, }; - // Nothing has changed, skipping update - if (deepEqual(this.context.user, newUserContext)) return; - this.context.user = newUserContext; - void this.user(); - - // Update the feedback user if the user ID has changed - if (userIdChanged) { - void this.updateAutoFeedbackUser(String(user.id)); - } - - await this.flagsClient.setContext(this.context); + return this.applyContext( + { + user: newUserContext, + company: this.context.company, + other: this.context.other, + }, + { warnOnMissingIds: false }, + ); } /** @@ -640,12 +636,14 @@ export class ReflagClient { id: company.id ?? this.context.company?.id, }; - // Nothing has changed, skipping update - if (deepEqual(this.context.company, newCompanyContext)) return; - this.context.company = newCompanyContext; - void this.company(); - - await this.flagsClient.setContext(this.context); + return this.applyContext( + { + user: this.context.user, + company: newCompanyContext, + other: this.context.other, + }, + { warnOnMissingIds: false }, + ); } /** @@ -663,11 +661,14 @@ export class ReflagClient { ...otherContext, }; - // Nothing has changed, skipping update - if (deepEqual(this.context.other, newOtherContext)) return; - this.context.other = newOtherContext; - - await this.flagsClient.setContext(this.context); + return this.applyContext( + { + user: this.context.user, + company: this.context.company, + other: newOtherContext, + }, + { warnOnMissingIds: false }, + ); } /** @@ -676,9 +677,15 @@ export class ReflagClient { * * @param context The context to update. */ - async setContext({ otherContext, ...context }: ReflagDeprecatedContext) { - const userIdChanged = - context.user?.id && context.user.id !== this.context.user?.id; + async setContext(context: ReflagDeprecatedContext) { + return this.applyContext(context, { warnOnMissingIds: true }); + } + + private async applyContext( + { otherContext, ...context }: ReflagDeprecatedContext, + { warnOnMissingIds }: { warnOnMissingIds: boolean }, + ) { + const previousContext = this.context; // Create a new context object making sure to clone the user and company objects const newContext = { @@ -687,32 +694,49 @@ export class ReflagClient { other: { ...otherContext, ...context.other }, }; - if (!context.user?.id) { + if (warnOnMissingIds && !context.user?.id) { this.logger.warn("No user Id provided in context, user will be ignored"); } - if (!context.company?.id) { + if (warnOnMissingIds && !context.company?.id) { this.logger.warn( "No company Id provided in context, company will be ignored", ); } // Nothing has changed, skipping update - if (deepEqual(this.context, newContext)) return; + if (deepEqual(previousContext, newContext)) return; + + const userChanged = !deepEqual(previousContext.user, newContext.user); + const companyChanged = !deepEqual( + previousContext.company, + newContext.company, + ); + const userIdChanged = + !!newContext.user?.id && newContext.user.id !== previousContext.user?.id; + this.context = newContext; - if (context.company) { + if (companyChanged) { void this.company(); } - if (context.user) { + if (userChanged) { void this.user(); // Update the automatic feedback user if the user ID has changed if (userIdChanged) { - void this.updateAutoFeedbackUser(String(context.user.id)); + void this.updateAutoFeedbackUser(String(newContext.user!.id)); } } - await this.flagsClient.setContext(this.context); + const shouldTrackLoading = this.state === "initialized"; + if (shouldTrackLoading) { + this.setState("initializing"); + } + + const didApply = await this.flagsClient.setContext(this.context); + if (didApply && this.state === "initializing") { + this.setState("initialized"); + } } /** diff --git a/packages/browser-sdk/src/flag/flags.ts b/packages/browser-sdk/src/flag/flags.ts index 1a776f6a..81ecf62b 100644 --- a/packages/browser-sdk/src/flag/flags.ts +++ b/packages/browser-sdk/src/flag/flags.ts @@ -206,6 +206,7 @@ export class FlagsClient { private flagOverrides: FlagOverrides = {}; private flags: RawFlags = {}; private fallbackFlags: FallbackFlags = {}; + private contextFetchVersion = 0; private storage: StorageAdapter; private refreshEvents: number[] = []; @@ -291,7 +292,15 @@ export class FlagsClient { async setContext(context: ReflagContext) { this.context = context; - this.setFetchedFlags((await this.maybeFetchFlags()) || {}); + const requestVersion = ++this.contextFetchVersion; + const fetchedFlags = (await this.maybeFetchFlags()) || {}; + + if (requestVersion !== this.contextFetchVersion) { + return false; + } + + this.setFetchedFlags(fetchedFlags); + return true; } updateFlags(triggerEvent = true) { diff --git a/packages/browser-sdk/test/client.test.ts b/packages/browser-sdk/test/client.test.ts index bf1fe228..17069b20 100644 --- a/packages/browser-sdk/test/client.test.ts +++ b/packages/browser-sdk/test/client.test.ts @@ -44,6 +44,20 @@ describe("ReflagClient", () => { }); expect(flagClientSetContext).toHaveBeenCalledWith(client["context"]); }); + + it("does not warn about a missing company when updating a user-only context", async () => { + client = new ReflagClient({ + publishableKey: "test-key-user-only", + user: { id: "user1" }, + }); + const warnSpy = vi.spyOn(client.logger, "warn"); + + await client.updateUser({ name: "Updated User" }); + + expect(warnSpy).not.toHaveBeenCalledWith( + "No company Id provided in context, company will be ignored", + ); + }); }); describe("updateCompany", () => { @@ -152,6 +166,101 @@ describe("ReflagClient", () => { expect(checkHook).not.toHaveBeenCalled(); expect(flagsUpdated).not.toHaveBeenCalled(); }); + + it("sets state to initializing while refetching flags after initialization", async () => { + await client.initialize(); + + let resolveFetch: (() => void) | undefined; + const setContextPromise = new Promise((resolve) => { + resolveFetch = () => resolve(true); + }); + const setContext = vi + .spyOn(FlagsClient.prototype, "setContext") + .mockImplementation(async () => { + return setContextPromise; + }); + + const stateUpdated = vi.fn(); + client.on("stateUpdated", stateUpdated); + + const updatePromise = client.updateOtherContext({ workspaceId: "ws-1" }); + + expect(client.getState()).toBe("initializing"); + expect(stateUpdated).toHaveBeenCalledWith("initializing"); + expect(setContext).toHaveBeenCalledWith(client["context"]); + + resolveFetch?.(); + await updatePromise; + + expect(client.getState()).toBe("initialized"); + expect(stateUpdated).toHaveBeenLastCalledWith("initialized"); + }); + + it("keeps loading tied to the latest context update", async () => { + await client.initialize(); + + let resolveFirstFetch: (() => void) | undefined; + let resolveSecondFetch: (() => void) | undefined; + const firstFetch = new Promise((resolve) => { + resolveFirstFetch = () => resolve(false); + }); + const secondFetch = new Promise((resolve) => { + resolveSecondFetch = () => resolve(true); + }); + + vi.spyOn(FlagsClient.prototype, "setContext") + .mockImplementationOnce(async () => firstFetch) + .mockImplementationOnce(async () => secondFetch); + + const firstUpdate = client.updateOtherContext({ workspaceId: "ws-1" }); + const secondUpdate = client.updateOtherContext({ workspaceId: "ws-2" }); + + expect(client.getState()).toBe("initializing"); + + resolveSecondFetch?.(); + await secondUpdate; + + expect(client.getState()).toBe("initialized"); + + resolveFirstFetch?.(); + await firstUpdate; + + expect(client.getState()).toBe("initialized"); + }); + }); + + describe("setContext warnings", () => { + it("does not warn about missing ids when updating anonymous other context", async () => { + client = new ReflagClient({ + publishableKey: "test-key-anon", + }); + const warnSpy = vi.spyOn(client.logger, "warn"); + + await client.updateOtherContext({ workspaceId: "ws-1" }); + + expect(warnSpy).not.toHaveBeenCalledWith( + "No user Id provided in context, user will be ignored", + ); + expect(warnSpy).not.toHaveBeenCalledWith( + "No company Id provided in context, company will be ignored", + ); + }); + + it("still warns when setContext replaces context without user or company ids", async () => { + client = new ReflagClient({ + publishableKey: "test-key-set-context", + }); + const warnSpy = vi.spyOn(client.logger, "warn"); + + await client.setContext({ other: { workspaceId: "ws-1" } }); + + expect(warnSpy).toHaveBeenCalledWith( + "No user Id provided in context, user will be ignored", + ); + expect(warnSpy).toHaveBeenCalledWith( + "No company Id provided in context, company will be ignored", + ); + }); }); describe("offline mode", () => { diff --git a/packages/browser-sdk/test/flags.test.ts b/packages/browser-sdk/test/flags.test.ts index b531f353..42d2575f 100644 --- a/packages/browser-sdk/test/flags.test.ts +++ b/packages/browser-sdk/test/flags.test.ts @@ -141,6 +141,75 @@ describe("FlagsClient", () => { expect(timeoutMs).toEqual(5000); }); + test("only applies flags from the latest context update", async () => { + const { newFlagsClient } = flagsClientFactory(); + const flagsClient = newFlagsClient(); + + let resolveFirstFetch: + | ((flags: Record) => void) + | undefined; + let resolveSecondFetch: + | ((flags: Record) => void) + | undefined; + const firstFetch = new Promise>((resolve) => { + resolveFirstFetch = resolve; + }); + const secondFetch = new Promise>((resolve) => { + resolveSecondFetch = resolve; + }); + + vi.spyOn(flagsClient as any, "maybeFetchFlags") + .mockImplementationOnce(async () => firstFetch) + .mockImplementationOnce(async () => secondFetch); + + const firstUpdate = flagsClient.setContext({ + user: { id: "user-1" }, + company: { id: "company-1" }, + other: { workspaceId: "workspace-1" }, + }); + const secondUpdate = flagsClient.setContext({ + user: { id: "user-2" }, + company: { id: "company-2" }, + other: { workspaceId: "workspace-2" }, + }); + + resolveSecondFetch?.({ + latestFlag: { + key: "latestFlag", + isEnabled: true, + targetingVersion: 2, + }, + }); + + await expect(secondUpdate).resolves.toBe(true); + expect(flagsClient.getFlags()).toEqual({ + latestFlag: { + key: "latestFlag", + isEnabled: true, + targetingVersion: 2, + isEnabledOverride: null, + }, + }); + + resolveFirstFetch?.({ + staleFlag: { + key: "staleFlag", + isEnabled: false, + targetingVersion: 1, + }, + }); + + await expect(firstUpdate).resolves.toBe(false); + expect(flagsClient.getFlags()).toEqual({ + latestFlag: { + key: "latestFlag", + isEnabled: true, + targetingVersion: 2, + isEnabledOverride: null, + }, + }); + }); + test("return fallback flags on failure (string list)", async () => { const { newFlagsClient, httpClient } = flagsClientFactory(); diff --git a/packages/react-sdk/src/index.tsx b/packages/react-sdk/src/index.tsx index 26a69b71..a287facd 100644 --- a/packages/react-sdk/src/index.tsx +++ b/packages/react-sdk/src/index.tsx @@ -5,6 +5,7 @@ import React, { ReactNode, useContext, useEffect, + useLayoutEffect, useMemo, useState, } from "react"; @@ -27,6 +28,9 @@ import { import { version } from "../package.json"; +const useIsomorphicLayoutEffect = + typeof window === "undefined" ? useEffect : useLayoutEffect; + export type { CheckEvent, CompanyContext, @@ -667,7 +671,7 @@ export function useOnEvent( `ReflagProvider is missing and no client was provided. Please ensure your component is wrapped with a ReflagProvider.`, ); } - useEffect(() => { + useIsomorphicLayoutEffect(() => { return resolvedClient.on(event, handler); }, [resolvedClient, event, handler]); } diff --git a/packages/react-sdk/test/usage.test.tsx b/packages/react-sdk/test/usage.test.tsx index 8b5d5301..084994a4 100644 --- a/packages/react-sdk/test/usage.test.tsx +++ b/packages/react-sdk/test/usage.test.tsx @@ -1,5 +1,6 @@ import React from "react"; -import { render, renderHook, waitFor } from "@testing-library/react"; +import { renderToString } from "react-dom/server"; +import { act, render, renderHook, waitFor } from "@testing-library/react"; import { http, HttpResponse } from "msw"; import { setupServer } from "msw/node"; import { @@ -1065,6 +1066,130 @@ describe("useIsLoading", () => { unmount(); }); + test("returns loading state while refetching flags after a context change", async () => { + function LoadingState() { + return {String(useIsLoading())}; + } + + const client = new ReflagClient({ + publishableKey: "test-key-loading-context-change", + user, + company, + other, + bootstrappedFlags: { + abc: { + key: "abc", + isEnabled: true, + targetingVersion: 1, + }, + }, + }); + await client.initialize(); + + const { getByTestId, unmount } = render( + + + , + ); + + await waitFor(() => { + expect(getByTestId("loading-state").textContent).toBe("false"); + }); + + await act(async () => undefined); + + act(() => { + (client as any).hooks.trigger("stateUpdated", "initializing"); + }); + + await waitFor(() => { + expect(getByTestId("loading-state").textContent).toBe("true"); + }); + + act(() => { + (client as any).hooks.trigger("stateUpdated", "initialized"); + }); + + await waitFor(() => { + expect(getByTestId("loading-state").textContent).toBe("false"); + }); + + unmount(); + }); + + test("returns loading state when ReflagProvider context updates", async () => { + function LoadingState() { + return {String(useIsLoading())}; + } + + const initializeSpy = vi + .spyOn(ReflagClient.prototype, "initialize") + .mockResolvedValue(undefined); + let resolveSetContext: (() => void) | undefined; + const setContextPromise = new Promise((resolve) => { + resolveSetContext = resolve; + }); + const setContextSpy = vi + .spyOn(ReflagClient.prototype, "setContext") + .mockResolvedValueOnce(undefined) + .mockImplementationOnce(async function () { + (this as any).hooks.trigger("stateUpdated", "initializing"); + await setContextPromise; + (this as any).hooks.trigger("stateUpdated", "initialized"); + }); + + const initialContext = { user, company, other }; + const updatedContext = { + ...initialContext, + other: { ...other, workspaceId: "workspace-1" }, + }; + + const { getByTestId, rerender, unmount } = render( + + + , + ); + + await waitFor(() => { + expect(getByTestId("loading-state").textContent).toBe("false"); + }); + + await waitFor(() => { + expect(setContextSpy).toHaveBeenCalledTimes(1); + }); + + act(() => { + rerender( + + + , + ); + }); + + await waitFor(() => { + expect(setContextSpy).toHaveBeenCalledTimes(2); + expect(getByTestId("loading-state").textContent).toBe("true"); + }); + + resolveSetContext?.(); + + await waitFor(() => { + expect(getByTestId("loading-state").textContent).toBe("false"); + }); + + unmount(); + initializeSpy.mockRestore(); + setContextSpy.mockRestore(); + }); + test("throws error when used outside provider", () => { const consoleErrorSpy = vi .spyOn(console, "error") @@ -1083,6 +1208,37 @@ describe("useIsLoading", () => { }); describe("useOnEvent", () => { + test("does not trigger the SSR useLayoutEffect warning when used inside a provider", () => { + const errorSpy = vi + .spyOn(console, "error") + .mockImplementation(() => undefined); + const client = new ReflagClient({ + publishableKey: "test-key-ssr", + user, + company, + other, + }); + + function EventListener() { + useOnEvent("flagsUpdated", vi.fn()); + return null; + } + + renderToString( + + + , + ); + + expect( + errorSpy.mock.calls.some(([firstArg]) => + String(firstArg).includes("useLayoutEffect"), + ), + ).toBe(false); + + errorSpy.mockRestore(); + }); + test("subscribes to flagsUpdated event", async () => { const eventHandler = vi.fn(); const client = new ReflagClient({