socketio / socket.io

Realtime application framework (Node.JS server)
https://socket.io
MIT License
61.25k stars 10.12k forks source link

Consider Opening the WebSocket Connection on pageshow and close it on beforeunload to allow BFCache #5232

Open gilbertococchi opened 4 days ago

gilbertococchi commented 4 days ago

Similar to this issue: https://github.com/socketio/socket.io/issues/1660

All pages using socket.io cannot benefit of BFCache on Chrome because of the way the WebSocket Connection is established/closed.

Many sites could benefit of this speed improvements, therefore it would be ideal if this library would be open to:

1) Open the WebSocket connection at pageshow. 2) Close the WebSocket connection using beforeunload.

2) should be done by using pagehide but there is a Chrome bug that would make the change not effective enough: https://g-issues.chromium.org/issues/360183659

darrachequesne commented 2 days ago

Hi!

Shouldn't it be the duty of the user of the library? Something like:

const socket = io({
  autoConnect: false
});

addEventListener("pageshow", (event) => {
  socket.connect();
});

addEventListener("beforeunload", (event) => {
  socket.disconnect();
});

Note: with the closeOnBeforeunload option set to true (defaults to false), the browser will automatically close the WebSocket connection when the beforeunload event is emitted.

gilbertococchi commented 1 day ago

Hi Damien, nice to e-meet you and thanks for your quick turn around here!

I agree with you that in theory it should be the developers using the library to use it in the proper way, but It's also likely that most of them may miss this point and sacrifice their page load performance.

Does the Socket.io documentation have a section where perhaps this practice can be suggested as best practice?

Something like, to make sure the WebSocket Connection would not prevent BFCache (that could be a big deal for Performance) make sure to call connect() on the pageshow event.

About the closeOnBeforeunload setting that you mention, is there a reason why the default is non true instead of false?

Actually, once this bug will be fixed, I would much rather suggest using pagehide instead of beforeunload for the same purpose but more reliable and less problematic event.