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

using mercure somehow breaks bfcache #696

Open
asdfzdfj opened this issue Apr 9, 2024 · 8 comments
Open

using mercure somehow breaks bfcache #696

asdfzdfj opened this issue Apr 9, 2024 · 8 comments

Comments

@asdfzdfj
Copy link
Contributor

asdfzdfj commented Apr 9, 2024

in short: mercure + bfcache + onbeforeunload => page cannot be stored in bfcache, causing every navigation to do a page load

possible solution: rewrite teardown function to either use pagehide event or stimulusjs disconnect() function

note that I might be off here, if anyone knows what's more likely them I'm all ears

EDIT 1: ok, maybe onbeforeunload isn't tha main culprit, it seems like having an open connection to mercure at all breaks bfcache, still needs more investigating


based on discussion/discovery here

You know what's great... It is mercure... I had it enabled and it is disabled on kbin.run
Now I have disabled it and no more extra requests...

Originally posted by @BentiGorlich in #689 (comment)

merde*, I think I have idea why:

  • this looks like a browser bfcache territory
  • the notification controller sets up onbeforeunload event listener to tear down the mercure connection
    connect() {
    this.es(this.getTopics());
    window.onbeforeunload = function (event) {
    if (window.es !== undefined) {
    window.es.close();
    }
    };
    }
  • this firefox docs says that the page is ineligible if beforeunload listener is used (the docs seems ancient but that's the only page of firefox bfcache docs I could find atm)

* somehow this sound came up in my head upon reading that

Originally posted by @asdfzdfj in #689 (comment)

@BentiGorlich
Copy link
Member

I just tried on my server changing window.onbeforeunload to window.onpagehide and now it works properly even on Fennec

@asdfzdfj
Copy link
Contributor Author

asdfzdfj commented Apr 9, 2024

on my own testing it's still some bit inconsistent, you sure that you didn't forgot to enable mercure back when testing?

@BentiGorlich
Copy link
Member

I am very sure that mercure was deactivated when testing, however Fennec still had the problems, because the notifications controller is always loaded, even when mercure is disabled

@BentiGorlich
Copy link
Member

But enabling mercure still breaks the cache, that's for sure 😁

@asdfzdfj
Copy link
Contributor Author

asdfzdfj commented Apr 9, 2024

yeah that seems to be it, it looks like the other half/part seems to be having mercure connection opened at all, so that's more to investigate

@BentiGorlich
Copy link
Member

I just tried enabling mercure, but have it not be reachable, so the request fails instantly. That solves the issue. But I think our code behaves differently when the connection is established versus not established... It is really a bummer that it doesn't just use web sockets, but this weird "eventsource" call...

@asdfzdfj asdfzdfj changed the title mercure teardown breaks bfcache using mercure somehow breaks bfcache Apr 9, 2024
@BentiGorlich
Copy link
Member

It is really a bummer that it doesn't just use web sockets, but this weird "eventsource" call...

Maybe I am wrong about it though. Seems like eventsources are way older and have wider support than websockets... Still weird

Copy link
Contributor

This issue is stale because it has been open 50 days with no activity. Remove stale label or comment or this will be closed in 6 days.

@github-actions github-actions bot added the Stale Inactivity for too long label May 30, 2024
@BentiGorlich BentiGorlich removed the Stale Inactivity for too long label Jun 6, 2024
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

No branches or pull requests

2 participants