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

Replace the deprecated unload event with pagehide #165

Closed
wants to merge 1 commit into from

Conversation

micrology
Copy link

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.

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
Copy link
Member

dmonad commented Jan 15, 2024

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
Copy link
Author

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
Copy link
Member

dmonad commented Jan 15, 2024

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 dmonad closed this Jan 15, 2024
@dmonad
Copy link
Member

dmonad commented Jan 15, 2024

Thanks for bringing this up!

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.

2 participants