pubnub / javascript

PubNub JavaScript SDK docs https://www.pubnub.com/docs/sdks/javascript
Other
553 stars 401 forks source link

feature: cleanup SDK on navigation event #421

Closed WalrusSoup closed 1 day ago

WalrusSoup commented 1 day ago

Cleanup on navigation event

Fixes https://github.com/pubnub/javascript/issues/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 commented 1 day ago

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 commented 1 day ago

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 commented 1 day ago

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 commented 1 day ago

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.