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(react-bindings): multiple client async start handling while loading #17

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nadavshatz
Copy link

No description provided.

@nadavshatz
Copy link
Author

One other thought I had was maybe we should move the loading being set into a useEffect. It might be consistent with react principles and safer for future changes.

Let me know if you agree and I can change it accordingly.

Didn't want to make a larger change without discussing first.

@daniel-statsig
Copy link
Contributor

Hey @nadavshatz, thanks for the PR. I don't quite follow what it is solving however.

initializeAsync is making a network request and then setting isLoading to false once it is resolved.

My understanding of the changes in this PR is that if a second call to useStatsigInternalClientFactoryAsync is made, it will just immediately resolve and set isLoading to false, regardless if the network request is still in flight.

@nadavshatz
Copy link
Author

For some reason on a second call or rerender (not sure the reason I just see a debugger being called again inside the use memo) then it just returns since the ref exists but the loading isn't being set since the code isn't reached for that instance. Maybe each call creates another instance of the is loading state.

@nadavshatz
Copy link
Author

After some more research it is indeed considered bad practice to modify state while in a useMemo.

Working with ChatGPT even provides this useEffect version which is much much safer.

I updated my PR to it and added to the test which actually fails with the old implementation.

@nadavshatz
Copy link
Author

lastly - we can use this if we don't want the strict mode wrapper.
https://testing-library.com/docs/react-testing-library/api/#reactstrictmode

I just didn't want to change a global setting.


useEffect(() => {
// already initializing or initialized
if (clientRef.current.loadingStatus !== 'Uninitialized') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you better outline what it is you are trying to solve? This change means that in strict mode, we will immediately render, regardless of the SDKs status.

Adding this logic means that we will just set isLoading to false on the re-render as its loadingStatus will be "Loading" and therefore pass this check.

Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is intended to prevent multiple SDK clients/network requests being created.

In React Strict mode, a re-render occurs, blowing away the previous state.
The logic here is supposed to create the client on the first render, then reuse it on the second.

Copy link
Author

Choose a reason for hiding this comment

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

Correct.
As far as I could tell, the old logic tried to do the same.

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

Successfully merging this pull request may close these issues.

2 participants