fix: close WebSocket on pagehide to support bfcache#162
fix: close WebSocket on pagehide to support bfcache#162
Conversation
| this.reconnectionAttempts = reconnectionAttempts; | ||
|
|
||
| this.pageHideHandler = () => { | ||
| log('Page entering bfcache, closing WebSocket', 'info'); |
There was a problem hiding this comment.
I don't think users will be happy to see this log in their application. Let's remove it after testing.
There was a problem hiding this comment.
Pull request overview
Updates the JavaScript Socket transport to close its WebSocket connection on pagehide, enabling the page to enter the browser back/forward cache (bfcache) and addressing Lighthouse warnings.
Changes:
- Adds a
pagehideevent handler that closes the WebSocket. - Registers/unregisters the
pagehidelistener when the socket opens/closes. - Introduces an internal
close()helper to null out the socket after closing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private setupListeners(): void { | ||
| window.addEventListener('pagehide', this.pageHideHandler, { capture: true }); | ||
| } |
There was a problem hiding this comment.
The new bfcache behavior (listening to pagehide and closing the WebSocket) isn’t covered by tests. Since this package already uses Vitest + jsdom, it should be possible to add a unit test that mocks WebSocket, dispatches a pagehide event (with persisted: true if you adopt that check), and asserts that close() is called and that a later send() recreates the connection.
| this.pageHideHandler = () => { | ||
| log('Page entering bfcache, closing WebSocket', 'info'); | ||
| this.close(); | ||
| }; |
There was a problem hiding this comment.
pageHideHandler always logs "Page entering bfcache" and closes the socket, but it doesn’t receive the pagehide event so it can’t check event.persisted. This makes the log message inaccurate and deviates from the MDN pattern (close only when the page is being persisted to bfcache). Consider typing the handler as (event: PageTransitionEvent) => void, checking event.persisted, and updating the log message accordingly (or only logging when persisted is true).
| * Create new WebSocket connection and setup event listeners | ||
| * Remove window event listeners | ||
| */ | ||
| public destroyListeners(): void { |
There was a problem hiding this comment.
destroyListeners() is declared public, but it appears to be an internal helper (only used inside this class). Exposing it expands the public API surface of Socket without being part of the Transport interface; consider making it private (or introducing a single public destroy() method that both removes listeners and closes the socket if external cleanup is intended).
| public destroyListeners(): void { | |
| private destroyListeners(): void { |
3 participants
Original problem
Lighthouse was giving an exception "Pages with WebSocket cannot enter back/forward cache (bfcache)" because we were not closing socket on
pagehideevent.PR Description
Solution from MDN reference was implemented. When reproducing notice that
vite devalso opens a socket, so, instead usenpx vite buildwithnpx serveand import changing inindex.html.pageshowevent is ignored, because socket connection will be restored when send method will be called.