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

feature: cleanup SDK on navigation event #421

Conversation

WalrusSoup
Copy link

Cleanup on navigation event

Fixes #398 by adding an optional window listener to the beforeunload event. Utilizes the destroy method in the SDK similar to the networkDownDetected function.

Alleviates runaway garbage collection in tested Safari verison Version 17.5 (19618.2.12.11.6)

Screenshot 2024-11-20 at 10 49 35 AM
Screenshot 2024-11-20 at 10 49 50 AM

Optional value as to not introduce breaking changes for any users.

Usage

Pubnub Chat users can utilize this function via the constructor:

Chat.init({
    // ... other options
    cleanupOnNavigationEvent: true // this flag
}).

@parfeon
Copy link
Contributor

parfeon commented Nov 20, 2024

Is there any reason to have code which cleans up an instance created by user right into SDK client code if it can be written by SDK user:

window.addEventListener('beforeunload', () => {
    pubnub.destroy(true);
});

@WalrusSoup
Copy link
Author

WalrusSoup commented Nov 20, 2024

It's my opinion that it falls into the scope of the SDK given the severity of the issue. If we're going to say that it's the responsibility of the caller, the same argument could be made for the online and offline handlers.

Additionally, only stop() is exposed via types - so users would have to call a deprecated function which will be removed rather than destroy - and stop() does not support the optional isOffline parameter. The chat-sdk only exposes a @readonly pubnub instance that pulls in those types.

edit: types are fixed for me at least by installing the pubnub/javascript alongside chat-js so we're good there.

I think this fix is surgical and provides the necessary nice to haves to work downstream with the chat-sdk (which is a convenience sdk to begin with) and not introduce any new issues. Feel free to close it if there is any disagreement, and I will close the open issue as well.

@parfeon
Copy link
Contributor

parfeon commented Nov 20, 2024

destroy is publicly available. Type from DefinitelyTyped not updated for 9 months and the library after all this time bundled with own types.

Reachability status change is something what needed to the library itself (to stop/pause processes), when suggested fix related to the environment clean up. I will ask team opinion on this change.

@WalrusSoup
Copy link
Author

It looks like, upon inspection, chrome is going to deprecate this feature and it's likely that safari would follow. So, i am not sure if this is going to be viable long term anyways. Closing this to find another approach.

@WalrusSoup WalrusSoup closed this Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Runaway GC in Safari\Webkit on refresh without disconnect()
2 participants