AxaFrance / oidc-client

Light, Secure, Pure Javascript OIDC (Open ID Connect) Client. We provide also a REACT wrapper (compatible NextJS, etc.).
MIT License
597 stars 160 forks source link

proper clean up #740

Closed pinggi closed 2 years ago

pinggi commented 2 years ago

Issue and Steps to Reproduce

Shouldn't be oidc.removeEventSubscription(subscriptionId); called in useEffect cleanup function here?

Versions.

"@axa-fr/react-oidc-context": "^4.5.8"

guillaume-chervet commented 2 years ago

Thank you for your issue @pinggi, you are right this is an issue thank you very much.

pinggi commented 2 years ago

@guillaume-chervet that's great you fixed it. I think you could have deleted also this code:

        if(subscriptionId){
            oidc.removeEventSubscription(subscriptionId);
        }
  1. when a component is mounted, useEffect function is called and the cleanup function returned from it registered
  2. when the useEffect dependencies are changed (in this case configuration or configurationName), the cleanup function is called first and then the useEffect function

I also think the if(isMounted) { check is also useless since it's not an async code that would run when the component is unmounted - it's indented like it would be part of the oidc.subscriveEvents function but in fact it isn't.

guillaume-chervet commented 2 years ago

Thank you again @pinggi , you are completly right again !

guillaume-chervet commented 2 years ago

Hi will make a PullRequest Tomorrow.

guillaume-chervet commented 2 years ago

Thnak you very much @pinggi, I close the PullRequest.