proxy-client-react icon indicating copy to clipboard operation
proxy-client-react copied to clipboard

Flag Provider Re-rendering problem

Open Criezc opened this issue 1 year ago • 3 comments

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 avatar Mar 07 '25 10:03 Criezc

@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?

Criezc avatar Mar 14 '25 04:03 Criezc

@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.

FredrikOseberg avatar Mar 18 '25 08:03 FredrikOseberg

@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.

Criezc avatar Mar 19 '25 08:03 Criezc

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.

Tymek avatar Apr 10 '25 11:04 Tymek

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.

Criezc avatar Apr 10 '25 11:04 Criezc

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?

Tymek avatar Apr 10 '25 11:04 Tymek

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.

Criezc avatar Apr 10 '25 12:04 Criezc

If we where to improve re-rendering for 'flagsRead' and 'flagsError', we would need to split provider into React Context Providers. That's a large refactor for a little improvement. This could be done in the future, but it's not a priority right now.

Tymek avatar May 06 '25 08:05 Tymek