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.

Sign up for GitHub

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

Do not render cache after preloaded response #458

Merged
merged 10 commits into from
Nov 22, 2021
Merged

Do not render cache after preloaded response #458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c787063
adding test for cache then render race condition
tleish Nov 19, 2021
9f397c4
move tests to visit_tests
tleish Nov 19, 2021
6639d9d
reverting last change
tleish Nov 19, 2021
df93fe8
do not load cache snapshot if reponse is preloaded
tleish Nov 19, 2021
d766960
fix firefox timeout test
tleish Nov 19, 2021
5795370
move redirect test to visit_tests
tleish Nov 19, 2021
edad4fc
fix loadCachedSnapshot method
tleish Nov 19, 2021
4c3ca76
fix failing tests
tleish Nov 19, 2021
31790a4
testing if failing tests in CI come from code changes
tleish Nov 19, 2021
7ac2db2
changing solution to order of callsin visitStarted
tleish Nov 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/core/native/browser_adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@ export class BrowserAdapter implements Adapter {
}

visitStarted(visit: Visit) {
visit.loadCachedSnapshot()
visit.issueRequest()
visit.changeHistory()
visit.goToSamePageAnchor()
visit.loadCachedSnapshot()
}

visitRequestStarted(visit: Visit) {
Expand Down
1 change: 1 addition & 0 deletions src/tests/fixtures/one.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ <h1>One</h1>
<!--styles ensure that the element will be scrolled to top when navigated to via an anchored link -->
<a name="named-anchor"></a>
<div id="element-id" style="margin-top: 1em; height: 200vh">An element with an ID</div>
<p><a id="redirection-link" href="/__turbo/redirect?path=/src/tests/fixtures/visit.html">Redirection link</a></p>

<turbo-frame id="navigate-top">
Replaced only the frame
Expand Down
15 changes: 15 additions & 0 deletions src/tests/functional/visit_tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,21 @@ export class VisitTests extends TurboDriveTestCase {
this.assert.isTrue(fetchResponseResult.responseHTML.indexOf('An element with an ID') > -1)
}

async "test cache does not override response after redirect"() {
await this.remote.execute(() => {
const cachedElement = document.createElement("some-cached-element")
document.body.appendChild(cachedElement)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@tleish tleish Nov 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@seanpdoyle seanpdoyle Nov 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

})

this.assert(await this.hasSelector("some-cached-element"))
this.clickSelector("#same-origin-link")
await this.nextBeat
this.clickSelector("#redirection-link")
await this.nextBeat // 301 redirect response
await this.nextBeat // 200 response
this.assert.notOk(await this.hasSelector("some-cached-element"))
}

async visitLocation(location: string) {
this.remote.execute((location: string) => window.Turbo.visit(location), [location])
}
Expand Down