search

yjs / y-websocket

Websocket Connector for Yjs
https://docs.yjs.dev/ecosystem/connection-provider/y-websocket
MIT License
492 stars 255 forks source link

Replace the deprecated unload event with pagehide #165

Closed micrology closed 7 months ago

micrology commented 7 months ago

The unloadHandler is called by an eventListener for the 'unload' event when the user moves away from the page being served by y-websocket. However, the unload event is now deprecated (see https://web.dev/articles/bfcache?utm_source=lighthouse&utm_medium=devtools#never-use-the-unload-event ) and the recommendation is to replace it with the 'pagehide' event. This PR does that, in both locations in y-websocket.js where it is used.

dmonad commented 7 months ago

Do you get some kind of error message or warning?

Technically, the current implementation is still fine (even if the unload event is deprecated and might not work anymore). It is simply an improvement to tell the server and connected peers that the user closed the website.

Currently, there is no other way to tell if the user definitely closed the website. And I might argue that we don't want to disconnect the websocket connection unless the user closes the website (not just click on another tab).

I see the argument with the bfcache. But does this implementation even work with bfcache? I.e. when the page is hidden, then the awareness won't work anymore.

micrology commented 7 months ago

Chrome's DevTools Lighthouse complains about the use of unload. While you are surely right that there is no way of definitively telling whether the user has "closed the website" (I suppose that means, knowing that the user has stopped using the web app), pagehide probably does a better job than unload and there are no downsides to the change that I know of.

dmonad commented 7 months ago

There is a downside. Once the awareness is marked as offline, there is no way to bring it back. If unload is called, there is no reasonable expectation that the user will come back. So we can safely mark that user as offline. If you run _unloadHandler every time the page is hidden, you will notice that the awareness is irrevocably destroyed and won't work anymore.

pagehide is not just a replacement for unload. It's a completely different thing.

I will remove the unload event as bfcache is probably more important. The server already detects if a client is disconnected and marks them as offline once that happens. However, local clients will still see that client lurking around for a time if they aren't connected to a server.

dmonad commented 7 months ago

Thanks for bringing this up!