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

Flag Provider Re-rendering problem #193

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

Flag Provider Re-rendering problem #193

Criezc opened this issue Mar 7, 2025 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@Criezc
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Investigating
Development

No branches or pull requests

2 participants