Skip to content

Flag Provider Re-rendering problem #193

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

Open
Criezc opened this issue Mar 7, 2025 · 7 comments
Open

Flag Provider Re-rendering problem #193

Criezc opened this issue Mar 7, 2025 · 7 comments
Assignees

Comments

@Criezc
Copy link
Contributor

Criezc commented Mar 7, 2025

Describe the bug

Hi there,

First, thank you for your work on this library - it's been really valuable for our project.

I've noticed what might be a performance issue with the FlagProvider component. During my debugging with React-Scan, I discovered that it's causing unnecessary re-renders in my application. The issue appears to be in the context value creation:

  const context = useMemo<IFlagContextValue>(
    () => ({
      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,
      setFlagsReady,
      setFlagsError,
    }),
    [flagsReady, flagsError]
  );

While the dependency array only includes flagsReady and flagsError, new function references are created each time this memo runs, which causes context consumers to re-render even when only using the stable methods.

Would it make sense to memoize these function references with useCallback to maintain stable references across renders?

Thanks for considering this feedback!

Steps to reproduce the bug

No response

Expected behavior

No response

Logs, error output, etc.

Screenshots

Image

Additional context

No response

Unleash version

"@unleash/proxy-client-react": "^4.2.4"

Subscription type

Open source

Hosting type

Self-hosted

SDK information (language and version)

No response

@Criezc Criezc added the bug Something isn't working label Mar 7, 2025
@FredrikOseberg FredrikOseberg self-assigned this Mar 11, 2025
@FredrikOseberg FredrikOseberg moved this from New to Investigating in Issues and PRs Mar 11, 2025
@Criezc
Copy link
Contributor Author

Criezc commented Mar 14, 2025

@FredrikOseberg Hi, I've tried to freeze several function before returned to context value, this solutions works to stop the reference changing across render.

It is a known issues that function reference unstable when you return it immediately without freezing it on React 🤞

  const on = useCallback(
    (event: string, callback: Function, ctx?: any) => client.current.on(event, callback, ctx),
    [],
  ) as IFlagContextValue['on']

  const off = useCallback(
    (event: string, callback?: Function) => client.current.off(event, callback),
    [],
  ) as IFlagContextValue['off']

  const isEnabled = useCallback((toggleName: string) => client.current.isEnabled(toggleName), [])

  const updateContext = useCallback(
    async (context: IMutableContext) => await client.current.updateContext(context),
    [],
  )

  const getVariant = useCallback((toggleName: string) => client.current.getVariant(toggleName), [])

Do you mind if I open up a PR to fix this issues?

@FredrikOseberg
Copy link
Contributor

@Criezc Apologies for the late response. No, I don't mind if you open a PR to fix this issue. I was attempting to reproduce before making a fix, but I've been pretty busy lately so I haven't gotten around to it yet. This seems like a reasonable fix so go ahead and I can review it when you put the PR up.

@Criezc
Copy link
Contributor Author

Criezc commented Mar 19, 2025

@Criezc Apologies for the late response. No, I don't mind if you open a PR to fix this issue. I was attempting to reproduce before making a fix, but I've been pretty busy lately so I haven't gotten around to it yet. This seems like a reasonable fix so go ahead and I can review it when you put the PR up.

Thank you very much, I've already opened a PR here.

Please kindly check it.

@Tymek
Copy link
Member

Tymek commented Apr 10, 2025

I did more digging. @Criezc how did you check that useCallback is working? I'm writing a test for it and I don't see the any difference in number of re-renders.

https://github.com/Unleash/proxy-client-react/pull/200/files#diff-f9c9be9da0e42d97a2e38f9759fded57b5ad4454754f49f01a9c4aace30b7f0dR269

Failing test for number of re-renders:
https://github.com/Unleash/proxy-client-react/actions/runs/14378846927/job/40317612783?pr=200

Any help is much appreciated. I'm trying to reproduce our setup and make sure this will not regress.

@Criezc
Copy link
Contributor Author

Criezc commented Apr 10, 2025

I did more digging. @Criezc how did you check that useCallback is working? I'm writing a test for it and I don't see the any difference in number of re-renders.

https://github.com/Unleash/proxy-client-react/pull/200/files#diff-f9c9be9da0e42d97a2e38f9759fded57b5ad4454754f49f01a9c4aace30b7f0dR269

Failing test for number of re-renders:
https://github.com/Unleash/proxy-client-react/actions/runs/14378846927/job/40317612783?pr=200

Any help is much appreciated. I'm trying to reproduce our setup and make sure this will not regress.

Hi, I'm using react-scan tools to detect what changing on my app (chat app). And I realized that every time messages incoming I've seen on react-scan that unleash provider triggering re-render on my whole app (which I didn't expect it to re-render the whole app). In this case I've checked too other provider is stable.

I'm proofing that using useCallback is the solution by copying it into my local code and use it. Then check it to verify it worked, and it is, the unleash provider no more re-rendering my whole app.

@Tymek
Copy link
Member

Tymek commented Apr 10, 2025

In my testing re-renders are caused by flagsReady and flagsError state in FlagProvider. I'm refactoring logic into useFlagsStatus. I'll check out React-scan. Any reproducible example will help. Do you update context when sending messages in this chat app?

@Criezc
Copy link
Contributor Author

Criezc commented Apr 10, 2025

In my testing re-renders are caused by flagsReady and flagsError state in FlagProvider. I'm refactoring logic into useFlagsStatus. I'll check out React-scan. Any reproducible example will help. Do you update context when sending messages in this chat app?

In my testing re-renders are caused by flagsReady and flagsError state in FlagProvider.

Thanks a lot for checking this out! Truly appreciate you taking the time to dig into this


I have just re-evaluated the situation using the latest version of React-Scan, and it appears that the FlagProvider itself is not re-rendering. It seems that the results I obtained earlier from React-Scan may have been a false positive, although I am uncertain how this occurred. I take responsibility for not double-checking it with my custom hook previously. I apologize for any confusion this may have caused.

Below is a screenshot from my custom hook, which demonstrates that the reference remains consistent after I interact with the chat application. No logs were generated following my interactions.

Image

Really appreciate you taking the time to look into it and clarify things.

I believe you are correct, the reference changes were due to the updates in flagsReady and flagsError, rather than as a result of my interactions with the application.

@Tymek Tymek added maybe later and removed bug Something isn't working labels Apr 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Investigating
Development

No branches or pull requests

3 participants