cyrilletuzi / angular-async-local-storage

Efficient client-side storage for Angular: simple API + performance + Observables + validation
MIT License
676 stars 68 forks source link

Performance: back/forward cache disabled with an open IndexedDB connection in Chromium #996

Open naveedahmed1 opened 1 year ago

naveedahmed1 commented 1 year ago

Checks before posting an issue

Configuration

"@angular/core": "16.0.0-next.7",


# Description of the issue

As described in https://web.dev/bfcache, the browsers' back/forward cache doesn't work when IndexedDB has an open connection. It seems that in current implementation the connections are opened but they're never closed. Here when running a Lighthouse audit for an Angular app, there's an error

Page prevented back/forward cache restoration 1 failure reason Many navigations are performed by going back to a previous page, or forwards again. The back/forward cache (bfcache) can speed up these return navigations. Learn more about the bfcache



The documentation of back/forward cache: https://developer.chrome.com/docs/lighthouse/performance/bf-cache/

# How to reproduce the issue

Running a Lighthouse audit for an Angular app using latest version (I am testing it on Version 112.0.5615.87 (Official Build) (64-bit)) of Chrome, should throw this error.
cyrilletuzi commented 1 year ago

Hi,

I have been able to reproduce the Lighthouse diagnostic in Chrome 112 (it was not here in previous versions).

Could you investigate if this is a Chrome-specific behavior? While disabling back/forward cache if an IndexedDB transaction is still opened makes sense, it is quite weird to disable it if a connection is still open, which is a very common and intended behavior.

The Chrome documentation itself is ambiguous, as it first tells there can be issues if "in the middle of an IndexedDB transaction", and then later in the article it tells there can be issues "with an open IndexedDB connection".

Firefox documentation says cache is disabled if "the page has running IndexedDB transactions".

Found nothing about indexedDb in Safari documentation.

Could you find a way to check the behavior of Firefox and Safari?

naveedahmed1 commented 1 year ago

Same is the case with Edge, in bfcache issue on lighthouse repository https://github.com/GoogleChrome/lighthouse/issues/13960 addyosmani mentioned about BFCache Not Restored Reasons

As per the list:

Pages that have an open IndexedDB connection are not currently eligible for back/forward cache.
Close the open connection in the pagehide event to make this page eligible.

I am not sure if this list is about all browsers.

There's some discussion on the topic in Firebase repo as well https://github.com/firebase/firebase-js-sdk/issues/6167

*Adding links for reference.

cyrilletuzi commented 1 year ago

Same is the case with Edge

Edge is based on Chromium so that was expected.

I've done some testing, and like the discussion in the Firebase thread you mentioned suggested, it is a big mess.

First, without any IndexedDB (or anything else that could disabled the back/forward cache):

Second, with IndexedDB opened:

So it is a non-standard Chromium behavior, and globally the implementation is buggy, so I am not sure the library will be able to fix this.

If anyone wants to investigate, be sure to launch your Angular project with ng serve --live-reload false, otherwise there is a WebSocket for live reload, and WebSockets also disable back/forward cache.

naveedahmed1 commented 1 year ago

Thank you so much @cyrilletuzi for your time and looking into this and sharing your findings. Yes I agree, but I am surprised that if its inconsistent across browsers, why have the Lighthouse team included it in latest version?

I have one question, can you please share what impact does the below workaround have on Angular app, when using this plugin:

    const open = window.indexedDB.open.bind(window.indexedDB)
    const dbs = []

    window.indexedDB.open = (...args) => {
      const request = open(...args)

      request.addEventListener('success', event => {
        const db = event.target.result
        dbs.push(db)
      })

      return request
    }

    window.addEventListener(
      'pagehide',
      () => {
        for (const db of dbs) {
          db.close()
        }
      },
      { capture: true },
    )
cyrilletuzi commented 1 year ago

why have the Lighthouse team included it in latest version?

Lighthouse is a part of Google Chrome, and its tests are opinionated. It could be an idea to report an issue in the Lighthouse repo to ask their reasoning, and what would be a cross-browser compatible solution.

can you please share what impact does the below workaround have on Angular app, when using this plugin

A console.log inside the loop of the pagehide event should be enough to see if it works or not. But be sure to test the behavior in all browsers. The last part your code could be implemented in the lib, but one other reason I said it may not be a good idea is because someone in the Firebase thread tell this:

On Safari for iOS v15.5, bfcache will sort of work regardless of closing IndexedDB connections or running deleteApp(), but there's a big problem: if you call deleteApp() on pagehide, when you come back to that page and it's restored from bfcache, a newly initialized Firestore instance will just not work anymore (e.g. if you add a document listener, onNext callback won't be called, neither with data from cache nor from network). Probably due to an incomplete cleanup. Now, if I let the page be hidden by Safari without calling deleteApp() and then call it on pageshow right before reinitializing Firebase, then it works. This is a serious bug and I think it should elevate the priority of this issue.