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

Communicate Visit direction with html[data-turbo-visit-direction] #1007

Merged
merged 9 commits into from
Dec 4, 2023
Merged

Communicate Visit direction with html[data-turbo-visit-direction] #1007

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
050fe38
Add direction detail to turbo:visit events
domchristie Sep 14, 2023
3111416
Test restorationIndex is persisted between reloads
domchristie Sep 14, 2023
c39c223
Add data-turbo-visit-direction to html element
domchristie Sep 28, 2023
badfd69
Versatile Visit direction API.
domchristie Oct 3, 2023
319adf2
Remove "test" prefix from test description
domchristie Nov 17, 2023
6912e88
Fix typos
domchristie Nov 17, 2023
2bc7c13
Remove "test" prefix from test description
domchristie Nov 17, 2023
70cda68
Clean up assertions
afcapel Nov 17, 2023
c6f82a5
Read the mutation logs to assert the direction attribute change
afcapel Nov 17, 2023
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
12 changes: 9 additions & 3 deletions src/core/drive/history.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export class History {
restorationData = {}
started = false
pageLoaded = false
currentIndex = 0

constructor(delegate) {
this.delegate = delegate
Expand All @@ -15,6 +16,7 @@ export class History {
if (!this.started) {
addEventListener("popstate", this.onPopState, false)
addEventListener("load", this.onPageLoad, false)
this.currentIndex = history.state?.turbo?.restorationIndex || 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't aware of this until today, but browsers persist the state between refreshes (🤯). This means this feature should still work after a reload (e.g. after assets are invalidated and a user taps Back)

Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL, fascinating.

this.started = true
this.replace(new URL(window.location.href))
}
Expand All @@ -37,7 +39,9 @@ export class History {
}

update(method, location, restorationIdentifier = uuid()) {
const state = { turbo: { restorationIdentifier } }
if (method === history.pushState) ++this.currentIndex

const state = { turbo: { restorationIdentifier, restorationIndex: this.currentIndex } }
method.call(history, state, "", location.href)
this.location = location
this.restorationIdentifier = restorationIdentifier
Expand Down Expand Up @@ -81,9 +85,11 @@ export class History {
const { turbo } = event.state || {}
if (turbo) {
this.location = new URL(window.location.href)
const { restorationIdentifier } = turbo
const { restorationIdentifier, restorationIndex } = turbo
this.restorationIdentifier = restorationIdentifier
this.delegate.historyPoppedToLocationWithRestorationIdentifier(this.location, restorationIdentifier)
const direction = restorationIndex > this.currentIndex ? "forward" : "back"
this.delegate.historyPoppedToLocationWithRestorationIdentifierAndDirection(this.location, restorationIdentifier, direction)
this.currentIndex = restorationIndex
}
}
}
Expand Down
10 changes: 9 additions & 1 deletion src/core/drive/visit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ export const SystemStatusCode = {
contentTypeMismatch: -2
}

export const Direction = {
advance: "forward",
restore: "back",
replace: "none"
}

export class Visit {
identifier = uuid() // Required by turbo-ios
timingMetrics = {}
Expand Down Expand Up @@ -65,7 +71,8 @@ export class Visit {
willRender,
updateHistory,
shouldCacheSnapshot,
acceptsStreamResponse
acceptsStreamResponse,
direction
} = {
...defaultOptions,
...options
Expand All @@ -83,6 +90,7 @@ export class Visit {
this.scrolled = !willRender
this.shouldCacheSnapshot = shouldCacheSnapshot
this.acceptsStreamResponse = acceptsStreamResponse
this.direction = direction || Direction[action]
}

get adapter() {
Expand Down
7 changes: 5 additions & 2 deletions src/core/session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,12 @@ export class Session {

// History delegate

historyPoppedToLocationWithRestorationIdentifier(location, restorationIdentifier) {
historyPoppedToLocationWithRestorationIdentifierAndDirection(location, restorationIdentifier, direction) {
if (this.enabled) {
this.navigator.startVisit(location, restorationIdentifier, {
action: "restore",
historyChanged: true
historyChanged: true,
direction
})
} else {
this.adapter.pageInvalidated({
Expand Down Expand Up @@ -185,6 +186,7 @@ export class Session {
visitStarted(visit) {
if (!visit.acceptsStreamResponse) {
markAsBusy(document.documentElement)
this.view.markVisitDirection(visit.direction)
}
extendURLWithDeprecatedProperties(visit.location)
if (!visit.silent) {
Expand All @@ -193,6 +195,7 @@ export class Session {
}

visitCompleted(visit) {
this.view.unmarkVisitDirection()
clearBusyState(document.documentElement)
this.notifyApplicationAfterPageLoad(visit.getTimingMetrics())
}
Expand Down
8 changes: 8 additions & 0 deletions src/core/view.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,14 @@ export class View {
}
}

markVisitDirection(direction) {
this.element.setAttribute("data-turbo-visit-direction", direction)
}

unmarkVisitDirection() {
this.element.removeAttribute("data-turbo-visit-direction")
}

async renderSnapshot(renderer) {
await renderer.render()
}
Expand Down
1 change: 1 addition & 0 deletions src/tests/fixtures/visit.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
<section>
<h1>Visit</h1>
<p><a id="same-origin-link" href="/src/tests/fixtures/one.html">Same-origin link</a></p>
<p><a id="same-origin-replace-link" href="/src/tests/fixtures/one.html" data-turbo-action="replace">Same-origin replace link</a></p>
<p><a id="same-origin-link-search-params" href="/src/tests/fixtures/one.html?key=value">Same-origin link with ?key=value</a></p>
<p><a id="sample-response" href="/src/tests/fixtures/one.html">Sample response</a></p>
<p><a id="same-page-link" href="/src/tests/fixtures/visit.html">Same page link</a></p>
Expand Down
50 changes: 50 additions & 0 deletions src/tests/functional/visit_tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
readEventLogs,
scrollToSelector,
visitAction,
waitUntilSelector,
willChangeBody
} from "../helpers/page"

Expand Down Expand Up @@ -220,6 +221,55 @@ test("test Visit with network error", async ({ page }) => {
await nextEventNamed(page, "turbo:fetch-request-error")
})

test("test Visit direction data attribute when clicking a link", async ({ page }) => {
domchristie marked this conversation as resolved.
Show resolved Hide resolved
await Promise.all([
waitUntilSelector(page, "[data-turbo-visit-direction='forward']")
.then(() => waitUntilSelector(page, "html:not([data-turbo-visit-direction])")),
page.click("#same-origin-link")
])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can write these tests without relying on Promise.all:

  page.click("#same-origin-link")

  await waitUntilSelector(page, "[data-turbo-visit-direction='forward']")
  await waitUntilNoSelector(page, "[data-turbo-visit-direction]")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I tried this and I don't this it worked because the attribute is added and removed quicker than the assertions can track.

If I recall correctly, in the time it takes for page.click to resolve, the attribute it already added and removed, so waitUntilSelector(page, "[data-turbo-visit-direction='forward']") never passes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also line 226-227 ensures the attribute is added and removed in the correct order

Copy link
Collaborator

Choose a reason for hiding this comment

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

@domchristie what do you think of a660c7b?

The trick is that you don't await for the page.click, so it runs in parallel with the first waitUntilSelector. Then we await for the two assertions to ensure they happen one after the other.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh, that works locally but seems very flaky on CI.

Copy link
Collaborator

@afcapel afcapel Nov 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think our best option is to use the mutation logs so we are not so dependent on the exact assertion timing. With the mutation log, even if the attribute is no longer present in the page, we can still assert that it was present at some point.

See https://github.com/hotwired/turbo/compare/2bc7c13c66b2a31c4c46c63819d4f7f052538e6f..5d4758969663e4633cf5c61eb9b2bb23cf0a77a9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think our best option is to use the mutation logs

This looks good to me 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool, @domchristie if you can push those changes to your branch I'll merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@afcapel I will do (I’ve just moved house, and will have a reliable internet connection next week)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you can push those changes to your branch I'll merge

@afcapel Done

})

test("test Visit direction detdata attribute when navigating back", async ({ page }) => {
domchristie marked this conversation as resolved.
Show resolved Hide resolved
await page.click("#same-origin-link")
await nextEventNamed(page, "turbo:load")
await Promise.all([
waitUntilSelector(page, "[data-turbo-visit-direction='back']")
.then(() => waitUntilSelector(page, "html:not([data-turbo-visit-direction])")),
page.goBack()
])
})

test("test Visit direction attribute when navigating forward", async ({ page }) => {
domchristie marked this conversation as resolved.
Show resolved Hide resolved
await page.click("#same-origin-link")
await nextEventNamed(page, "turbo:load")
await page.goBack()
await nextEventNamed(page, "turbo:load")
await Promise.all([
waitUntilSelector(page, "[data-turbo-visit-direction='forward']")
.then(() => waitUntilSelector(page, "html:not([data-turbo-visit-direction])")),
page.goForward()
])
})

test("test Visit direction attribute on a replace visit", async ({ page }) => {
domchristie marked this conversation as resolved.
Show resolved Hide resolved
await Promise.all([
waitUntilSelector(page, "[data-turbo-visit-direction='none']")
.then(() => waitUntilSelector(page, "html:not([data-turbo-visit-direction])")),
page.click("#same-origin-replace-link")
])
})

test("test Turbo history state after a reload", async ({ page }) => {
domchristie marked this conversation as resolved.
Show resolved Hide resolved
await page.click("#same-origin-link")
await nextEventNamed(page, "turbo:load")
await page.reload()
assert.equal(
await page.evaluate(() => window.history.state.turbo.restorationIndex),
1,
"restorationIndex is persisted between reloads"
)
})

async function visitLocation(page, location) {
return page.evaluate((location) => window.Turbo.visit(location), location)
}