Cache snapshots synchronously

[stgm]

This is a bit of a cheeky way to report a bug, but this change works for me, so I thought I'd propose it.

I have a div marked as data-turbo-permanent. The contents of this div are not changed at any time when testing, it's just marked as permanent. The div is not contained inside a turbo-frame.

<div id="sidebar-content" data-turbo-permanent>
  ...
</div>

On my M1 Mac, I've got a problem in Safari. When clicking a link to visit a new page, the div is correctly cached and restored at all times. However, when going back/forth in the browser, more often than not, some of the pages will lose the sidebar contents and have the placeholder meta tag instead. From that point on, such a page will never have the sidebar content again. Firefox doesn't show this problem.

From some logging, it appears that in some cases, the snapshot is cached right after moving the permanent elements out of the DOM. Hence, removing the async flag from View.cacheSnapshot() is one way to resolve this problem.

Now this async flag is new in Turbo---in Turbolinks, the analogous part wasn't marked as async and I can confirm that I had never previously encountered this problem. This might indicate that it is indeed the culprit?

[sstephenson]

Now this async flag is new in Turbo---in Turbolinks, the analogous part wasn't marked as async and I can confirm that I had never previously encountered this problem. This might indicate that it is indeed the culprit?

Snapshot caches have been stored asynchronously since 0bd5e78 (May 2018). The recent change just converts this from a setTimeout callback to a promise.

Note that the snapshot itself is captured synchronously. It's the process of saving it that's async. We do this so other scripts on the page have a chance to tear down the page before it goes into cache. Full story here: turbolinks/turbolinks#390

[stgm]

Thanks! As you noticed, I'm a little bit in over my head, but I hope you can indulge me, because I can still consistently reproduce this problem and I've investigated a bit further.

I did notice just now that it ususally surfaces when I navigate to a page that has a Youtube/Vimeo embed. The page that I navigated away from is then cached with the permanent element replaced by a meta tag, which is revealed when navigating back.

From what I now understand, the problem could be the logic of the loadResponse() function, where cacheSnapshot() is started and immediately View.render() is called, which in turn will use the "current" state of the DOM to take out permanent elements.

loadResponse() {
  if (this.response) {
    const {statusCode: statusCode, responseHTML: responseHTML} = this.response;
    this.render((() => {
      this.cacheSnapshot();                                             <--- this is ASYNC
      if (isSuccessful(statusCode) && responseHTML != null) {
        this.view.render({
          snapshot: Snapshot.fromHTMLString(responseHTML)
        }, this.performScroll);
        ...
      }
    }));
  }
}

(The same logic is present in loadCachedSnapshot() but I didn't check out that case.)

That the timing is left up to chance (to some extent) seems to be confirmed by some call logging in the console---in my use case cacheSnapshot() only actually clones the DOM until after the permanent elements are relocated:

@ Snapshot.fromHTMLElement() (turbo.js, line 1138)
@ Visit.loadResponse() (turbo.js, line 1344)
@ View.cacheSnapshot() -> start (turbo.js, line 2363)
@ SnapshotRenderer.replaceBody() -> starting relocateCurrentBodyPermanentElements() (turbo.js, line 2224)
@ SnapshotRenderer.replaceBody() -> just finished relocateCurrentBodyPermanentElements() (turbo.js, line 2226)
@ View.cacheSnapshot() -> right before putting into snapShotCache (turbo.js, line 2369)
@ Snapshot.clone() (turbo.js, line 1148)
@ Snapshot.fromHTMLElement() (turbo.js, line 1138)
@ View.cacheSnapshot() -> DONE (turbo.js, line 2371)

From experimenting further, it appears that removing await nextMicrotask(); suffices for my use case on my laptop, but I have no idea about any further implications.

Let me know if I can/should convert this comment to an open issue, or change the PR.

[stgm]

@sstephenson Now that I understand it better I can see that you were already ahead of me with regards to the diagnosis.

However, I can't find any code that guarantees that the permanent elements are in the DOM when the snapshot is cached. No matter how I look at it, I always come back to that Snapshot.clone() that seems to be the point where the live DOM is actually copied, in my case right after the permanent elements have been replaced by meta tags.

[sstephenson]

@stgm Would you mind opening a separate issue with as specific of a diagnosis as you're able to make?

[stgm]

Just did, after reading up about the not-so-asyncness of JS :-)