Do not render cache after preloaded response

[tleish]

Fix a race condition where BrowserAdapter#visitStarted renders cache after preloaded content is rendered.

The BrowserAdapter has the following:

export class BrowserAdapter implements Adapter {
  //...
  visitStarted(visit) {
    visit.issueRequest() //=> Async send request
    visit.changeHistory()
    visit.goToSamePageAnchor()
    visit.loadCachedSnapshot() //=> render cached snapshot while waiting for async request
  }

When visit.issueRequest() takes longer than 1ms, then visit.loadCachedSnapshot() renders the cache first. When it renders immediately (as in a preloaded response) then visit.loadCachedSnapshot() renders last.

Fixes #447

[tleish]

@seanpdoyle - can you review this PR and let me know if I need to change anything?

[seanpdoyle]

Is it significant that this is an appended element? Could an <input> with text typed into it suit this test as state to preserve?

[tleish]

In my experience, the browser stores <input> text in memory, not in the DOM. Since turbo only caches DOM, testing cache through text input would not work.

Alternatively, I could add (or remove) a attribute to an existing DOM item (e.g. data-cached="true")

Essentially, I have to mock a scenario where the same URL is different from the first cached visit, to the follow-up new visit. Something like a <div id="flash">One Item Flash Message</div>.

[tleish]

@seanpdoyle - thumbs up means you are good with the explanation or you want an update to the test with one of the suggestions?

[seanpdoyle]

The current version works for me. Thank you for uncovering and digging into this!

[ghiculescu]

@tleish do you use the iOS adapter by any chance?

I'm trying to work out if this function has the same problem that this PR fixed: https://github.com/hotwired/turbo-ios/blob/fadb1f3888427a839f1e102ec5f38125f96172ba/Source/WebView/turbo.js#L116

[tleish]

@ghiculescu Yes, we do use the iOS adapter. What issue are you experiencing?

[ghiculescu]

It was a screen flicker, but it ended up being unrelated and due to us not creating a new view controller on each visit - apparently yiu really need to do that. So not this issue at all.

That said... does that code I linked look like it could cause the issue this PR addresses?

[tleish]

That said... does that code I linked look like it could cause the issue this PR addresses?

I haven't tested this particular bug on mobile, but it looks to follow the same older pattern and quite possibly has the same bug. Are you going to submit a PR to suggest an update to follow the same pattern as this MR?

[ghiculescu]

I can, but I'm not confident it's an issue yet. Do you have a test page I can use to replicate it?

[tleish]

@ghiculescu - See the steps to reproduce and demo app linked in the original #447 issue