Skip to content
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

Merged
merged 12 commits into from
Jun 28, 2024
Merged

fix: provider initialization #169

merged 12 commits into from
Jun 28, 2024

Conversation

Tymek
Copy link
Member

@Tymek Tymek commented Jun 13, 2024

There was a bug where the flagsReady status, returned from the useFlagsStatus hook, remains false even when an already started Unleash client is passed to the Unleash flag provider. This occurred despite the client being initialized and ready.

Closes #168

About the changes

  • test showing the issue
  • externalizing SDK state
    • Upgrade unleash-proxy-client dependency to version 3.5.1.
  • check state in Provider
    • Updated the initialization logic to check if the client is already ready and set flagsReady appropriately.

@Tymek Tymek marked this pull request as ready for review June 26, 2024 13:37
@@ -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?.()
Copy link
Contributor

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?

Copy link
Member Author

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

client.current.off('recovered', clearErrorCallback)
client.current.stop();
client.current.off('recovered', clearErrorCallback);
if (stopClient) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need a stopClient property?

Copy link
Member Author

Choose a reason for hiding this comment

The 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

Copy link

@cemreyavuz cemreyavuz Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you will probably also not want it to stop when React part of the app stops - in microfrontends for example

(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 ➕

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Thank you for clarifying.

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@FredrikOseberg FredrikOseberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Flags status is incorrect when an already started unleash client is passed to flag provider
3 participants