fix: useEffect cleanup #39
Conversation
| React.useEffect(() => { | ||
| client.current.start(); | ||
|
|
||
| return () => { |
There was a problem hiding this comment.
Add client.current.stop here too?
There was a problem hiding this comment.
Makes sense. Could move the client.current.on calls into useEffect as well.
|
Hi, looking forward for this fix to be merged, I agree that |
| React.useEffect(() => { | ||
| client.current.start(); | ||
|
|
||
| return () => { |
There was a problem hiding this comment.
Makes sense. Could move the client.current.on calls into useEffect as well.
| client.off('update'); | ||
| client.off('ready'); |
There was a problem hiding this comment.
Should we pass in the handlers here, to match the on calls above? Same for useVariant.
There was a problem hiding this comment.
Yeah I was a bit unsure, but the tinyEmitter library says you can omit these: https://www.npmjs.com/package/tiny-emitter
There was a problem hiding this comment.
It looks to me like if the handlers are not passed, then all of the event handlers on the client will be removed for those events. This would mean that existing event handlers on a client passed into the FlagProvider would be reset when the FlagProvider is unmounted.
There was a problem hiding this comment.
It looks to me like if the handlers are not passed, then all of the event handlers on the client will be removed for those events. This would mean that existing event handlers on a client passed into the
FlagProviderwould be reset when theFlagProvideris unmounted.
Yeah, I agree with your assessment, and we will add the event handlers to the the off calls.
|
Maybe I should open a new issue for this (please let me know if I should!) but I'd like to make a case for only starting and stopping the unleash client if it is created via config. That would allow a custom client passed in to One use case is delaying starting the client until custom properties of the unleash context are set. In order to get the initial query for flags to use these custom properties we would need to delay mounting the Please let me know what you think and if I should expand in a new issue :) |
I think this at least will require another PR. Can you open an issue? I can see your use-case, but we'll need to think a bit on how to implement this as it would be a breaking change for anyone using the current approach. Might have to be a setting if we want to avoid a bump in the major version. If you have any other ideas, we can discuss them in the issue - also happy to receive a PR for this. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Don't close. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Reopen. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@FredrikOseberg : what's the status on this? Any chance of it getting picked up? |
|
Superseded by #96 |
5 participants
No description provided.