-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: provider initialization #169
Changes from all commits
1a22d9a
ba95fea
934371a
046014a
aa6d36a
570062b
a3229c6
30acd87
aba7525
f69a6b8
a030bc6
9c4300c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,14 @@ | ||
/** @format */ | ||
|
||
import * as React from 'react'; | ||
import { IConfig, UnleashClient } from 'unleash-proxy-client'; | ||
import FlagContext, { IFlagContextValue } from './FlagContext'; | ||
import React, { type FC, type PropsWithChildren, useEffect, useMemo, useState } from 'react'; | ||
import { type IConfig, UnleashClient } from 'unleash-proxy-client'; | ||
import FlagContext, { type IFlagContextValue } from './FlagContext'; | ||
|
||
export interface IFlagProvider { | ||
config?: IConfig; | ||
unleashClient?: UnleashClient; | ||
startClient?: boolean; | ||
stopClient?: boolean; | ||
} | ||
|
||
const offlineConfig: IConfig = { | ||
|
@@ -24,11 +25,12 @@ const _startTransition = 'startTransition'; | |
// fallback for React <18 which doesn't support startTransition | ||
const startTransition = React[_startTransition] || (fn => fn()); | ||
|
||
const FlagProvider: React.FC<React.PropsWithChildren<IFlagProvider>> = ({ | ||
const FlagProvider: FC<PropsWithChildren<IFlagProvider>> = ({ | ||
config: customConfig, | ||
children, | ||
unleashClient, | ||
startClient = true, | ||
stopClient = true, | ||
}) => { | ||
const config = customConfig || offlineConfig; | ||
const client = React.useRef<UnleashClient>( | ||
|
@@ -37,13 +39,13 @@ const FlagProvider: React.FC<React.PropsWithChildren<IFlagProvider>> = ({ | |
const [flagsReady, setFlagsReady] = React.useState( | ||
Boolean( | ||
unleashClient | ||
? customConfig?.bootstrap && customConfig?.bootstrapOverride !== false | ||
? (customConfig?.bootstrap && customConfig?.bootstrapOverride !== false) || unleashClient.isReady?.() | ||
: config.bootstrap && config.bootstrapOverride !== false | ||
) | ||
); | ||
const [flagsError, setFlagsError] = React.useState(null); | ||
const [flagsError, setFlagsError] = useState(client.current.getError?.() || null); | ||
|
||
React.useEffect(() => { | ||
useEffect(() => { | ||
if (!config && !unleashClient) { | ||
console.error( | ||
`You must provide either a config or an unleash client to the flag provider. | ||
|
@@ -54,17 +56,17 @@ const FlagProvider: React.FC<React.PropsWithChildren<IFlagProvider>> = ({ | |
|
||
const errorCallback = (e: any) => { | ||
startTransition(() => { | ||
setFlagsError(currentError => currentError || e); | ||
setFlagsError((currentError: any) => currentError || e); | ||
}); | ||
}; | ||
|
||
const clearErrorCallback = (e: any) => { | ||
startTransition(() => { | ||
setFlagsError(null); | ||
}); | ||
} | ||
} | ||
|
||
let timeout: any; | ||
let timeout: ReturnType<typeof setTimeout> | null = null; | ||
const readyCallback = () => { | ||
// wait for flags to resolve after useFlag gets the same event | ||
timeout = setTimeout(() => { | ||
|
@@ -90,37 +92,24 @@ const FlagProvider: React.FC<React.PropsWithChildren<IFlagProvider>> = ({ | |
if (client.current) { | ||
client.current.off('error', errorCallback); | ||
client.current.off('ready', readyCallback); | ||
client.current.off('recovered', clearErrorCallback) | ||
client.current.stop(); | ||
client.current.off('recovered', clearErrorCallback); | ||
if (stopClient) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need a stopClient property? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you're passing an initialized client, you will probably also not want it to stop when React part of the app stops - in microfrontends for example There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
(Sorry for jumping to the discussion) This is exactly what I was trying to achieve before creating the referenced issue - so I can say that this is an actual use case for sure ➕ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. Thank you for clarifying. |
||
client.current.stop(); | ||
} | ||
} | ||
if (timeout) { | ||
clearTimeout(timeout); | ||
} | ||
}; | ||
}, []); | ||
|
||
const updateContext: IFlagContextValue['updateContext'] = async (context) => { | ||
await client.current.updateContext(context); | ||
}; | ||
|
||
const isEnabled: IFlagContextValue['isEnabled'] = (toggleName) => { | ||
return client.current.isEnabled(toggleName); | ||
}; | ||
|
||
const getVariant: IFlagContextValue['getVariant'] = (toggleName) => { | ||
return client.current.getVariant(toggleName); | ||
}; | ||
|
||
const on: IFlagContextValue['on'] = (event, callback, ctx) => { | ||
return client.current.on(event, callback, ctx); | ||
}; | ||
|
||
const context = React.useMemo<IFlagContextValue>( | ||
const context = useMemo<IFlagContextValue>( | ||
() => ({ | ||
on, | ||
updateContext, | ||
isEnabled, | ||
getVariant, | ||
on: ((event, callback, ctx) => client.current.on(event, callback, ctx)) as IFlagContextValue['on'], | ||
off: ((event, callback) => client.current.off(event, callback)) as IFlagContextValue['off'], | ||
updateContext: async (context) => await client.current.updateContext(context), | ||
isEnabled: (toggleName) => client.current.isEnabled(toggleName), | ||
getVariant: (toggleName) => client.current.getVariant(toggleName), | ||
client: client.current, | ||
flagsReady, | ||
flagsError, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,120 @@ | ||
import React from 'react'; | ||
import { render, screen, waitFor } from '@testing-library/react'; | ||
import useFlagsStatus from './useFlagsStatus'; | ||
import FlagProvider from './FlagProvider'; | ||
import { UnleashClient } from 'unleash-proxy-client'; | ||
|
||
const TestComponent = () => { | ||
const { flagsReady } = useFlagsStatus(); | ||
|
||
return <div>{flagsReady ? 'flagsReady' : 'loading'}</div>; | ||
}; | ||
|
||
const ErrorTestComponent = () => { | ||
const { flagsError } = useFlagsStatus(); | ||
|
||
return <div>{flagsError ? 'flagsError' : 'no issue'}</div>; | ||
}; | ||
|
||
|
||
const mockClient = { | ||
on: vi.fn(), | ||
off: vi.fn(), | ||
start: vi.fn(), | ||
stop: vi.fn(), | ||
updateContext: vi.fn(), | ||
isEnabled: vi.fn(), | ||
getVariant: vi.fn(), | ||
isReady: vi.fn(), | ||
} as unknown as UnleashClient; | ||
|
||
test('should initialize', async () => { | ||
const onEventHandler = (event: string, callback: () => void) => { | ||
if (event === 'ready') { | ||
callback(); | ||
} | ||
}; | ||
|
||
mockClient.on = onEventHandler as typeof mockClient.on; | ||
|
||
const ui = ( | ||
<FlagProvider unleashClient={mockClient}> | ||
<TestComponent /> | ||
</FlagProvider> | ||
); | ||
|
||
render(ui); | ||
|
||
await waitFor(() => { | ||
expect(screen.queryByText('flagsReady')).toBeInTheDocument(); | ||
}); | ||
}); | ||
|
||
// https://github.com/Unleash/proxy-client-react/issues/168 | ||
test('should start when already initialized client is passed', async () => { | ||
const client = new UnleashClient({ | ||
url: 'http://localhost:4242/api', | ||
fetch: async () => | ||
new Promise((resolve) => { | ||
setTimeout(() => | ||
resolve({ | ||
status: 200, | ||
ok: true, | ||
json: async () => ({ | ||
toggles: [], | ||
}), | ||
headers: new Headers(), | ||
}) | ||
); | ||
}), | ||
clientKey: '123', | ||
appName: 'test', | ||
}); | ||
await client.start(); | ||
expect(client.isReady()).toBe(true); | ||
|
||
const ui = ( | ||
<FlagProvider unleashClient={client}> | ||
<TestComponent /> | ||
</FlagProvider> | ||
); | ||
|
||
render(ui); | ||
|
||
await waitFor(() => { | ||
expect(screen.queryByText('flagsReady')).toBeInTheDocument(); | ||
}); | ||
}); | ||
|
||
test('should handle client errors', async () => { | ||
const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); | ||
|
||
const client = new UnleashClient({ | ||
url: 'http://localhost:4242/api', | ||
fetch: async () => { | ||
throw new Error('test error'); | ||
}, | ||
clientKey: '123', | ||
appName: 'test', | ||
}); | ||
|
||
await client.start(); | ||
|
||
const ui = ( | ||
<FlagProvider unleashClient={client}> | ||
<ErrorTestComponent /> | ||
</FlagProvider> | ||
); | ||
|
||
render(ui); | ||
|
||
await waitFor(() => { | ||
expect(screen.queryByText('flagsError')).toBeInTheDocument(); | ||
}); | ||
|
||
expect(consoleError).toHaveBeenCalledWith( | ||
'Unleash: unable to fetch feature toggles', | ||
expect.any(Error) | ||
); | ||
consoleError.mockRestore(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this the only significant line here? Why do we need these other changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's about adding the ability to pass initialized client. Error state is also needed to make sure state kept in React SDK is matching the one from initialized client