Tear Down client when Split Factory Component Unmounts

[drewmendenhall]

React SDK

What did you accomplish?

If / when the component unmounts, the client (event emitter) is leaked. This change cleans up the client on unmount.

How do we test the changes introduced in this PR?

Would a spy on client.destroy() suffice? I have made an attempt, please see the split-out fixup commit. I will give it another shot soon, but you might be able to help quicker.

Extra Notes

The larger issue is that there are cases where the id prop changes. Example scenario: a user begins unauthenticated, and is given a temporary split id. The use logs in and now their split id can be set to an id the application can identify the user with.

For a SPA, the SplitFactory element remains mounted, but a new id is passed. If I understand correctly, this component does not react to any prop changes once mounted. So a stop-gap solution is to set the element's key to the same as id to tell (or trick) React to un-mount the existing SplitFactory instance and mount a new one with the new id. This change is a good idea regardless. componentDidUpdate (or a hooks migration) can be implemented at a later time.

[nathanredblur]

This is even more than a fix, is a good practice destroy your listeners on destroy the component.

[NicoZelaya]

Hey @drewmendenhall !

Thank you very much for submitting the PR! our apologies for dropping the ball on replying to you. I went ahead and scheduled this to be picked up next week from our end, we'll let you know here how it goes.

@nathanredblur For the time being, maybe you can get around it by getting the client through the useClient hook to access that instance and invoking destroy() on a wrapping component unmount?
That is if this is giving you pain now, otherwise we'll have an update by end of next week most likely.

Best,
Nico.

[EmilianoSanchez]

LGTM

[EmilianoSanchez]

Thank you @drewmendenhall ,

We will merge your PR into development branch to include your contribution on next release.

[drewmendenhall]

@EmilianoSanchez would it help if I rebased to squash the fixup commit? or did you already plan on that?

edit after reviewing that commit: looks like it might be better to un-pick it and write a better test 😄

[EmilianoSanchez]

@EmilianoSanchez would it help if I rebased to squash the fixup commit? or did you already plan on that?

No. It is okey.

edit after reviewing that commit: looks like it might be better to un-pick it and write a better test 😄

Yes. Actually, we are updating some tests and polishing the code. So, probably your test will be modified a little bit. I will handle that :)