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

Propose a replace visit on redirection #328

Merged
merged 4 commits into from
Aug 24, 2021
Merged

Propose a replace visit on redirection #328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f1bfbdd
Propose a `replace` visit instead of directly modifying the history a…
Jul 30, 2021
1eb7bbe
Added navigation tests for redirection
Aug 8, 2021
e984a3f
Merge branch 'main' into replace-on-redirect
Aug 8, 2021
c292073
Merge branch 'main' into replace-on-redirect
dhh Aug 24, 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
7 changes: 5 additions & 2 deletions src/core/drive/visit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ export class Visit implements FetchRequestDelegate {
this.state = VisitState.completed
this.adapter.visitCompleted(this)
this.delegate.visitCompleted(this)
this.followRedirect()
}
}

Expand Down Expand Up @@ -259,8 +260,10 @@ export class Visit implements FetchRequestDelegate {

followRedirect() {
if (this.redirectedToLocation && !this.followedRedirect) {
this.location = this.redirectedToLocation
this.history.replace(this.redirectedToLocation, this.restorationIdentifier)
this.adapter.visitProposedToLocation(this.redirectedToLocation, {
action: 'replace',
response: this.response
})
this.followedRedirect = true
}
}
Expand Down
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 @@ -56,7 +56,7 @@ export class BrowserAdapter implements Adapter {
}

visitCompleted(visit: Visit) {
visit.followRedirect()

}

pageInvalidated() {
Expand Down
1 change: 1 addition & 0 deletions src/tests/fixtures/navigation.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ <h1>Navigation</h1>
<svg width="600" height="100" viewbox="-300 -50 600 100"><text><a id="cross-origin-link-inside-svg-element" href="about:blank">Cross-origin link inside SVG element</a></text></svg>
<p><a id="link-to-disabled-frame" href="/src/tests/fixtures/frames/hello.html" data-turbo-frame="hello">Disabled turbo-frame</a></p>
<p><a id="autofocus-link" href="/src/tests/fixtures/autofocus.html">autofocus.html link</a></p>
<p><a id="redirection-link" href="/__turbo/redirect?path=/src/tests/fixtures/one.html">Redirection link</a></p>
<p><a id="headers-link" href="/__turbo/headers">Headers link</a></p>
</section>

Expand Down
2 changes: 1 addition & 1 deletion src/tests/functional/form_submission_tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export class FormSubmissionTests extends TurboDriveTestCase {

this.assert.notOk(await this.formSubmitted)
this.assert.equal(await this.pathname, "/src/tests/fixtures/one.html")
this.assert.equal(await this.visitAction, "advance")
this.assert.equal(await this.visitAction, "replace")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is replace the expected action for GET form submissions?

It looks like we'll want a new test to validate normal visit redirects result in a new visit proposal with a replace action.

Copy link
Contributor Author

@jaysson jaysson Aug 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The sequence:

  • Visit the original URL with advance action
  • If there is a redirection, propose replace visit to the destination with the existing response. That way it doesn't fire one more fetch call, instead reuses the existing response.

User will see the same behaviour as before. Just that programmatically the sequence looks like advance + replace instead of just advance.

As long as there is a redirect, replace action is expected at the end. In this test, form is submitted to /__turbo/redirect (ref: https://github.com/jaysson/turbo/blob/replace-on-redirect/src/tests/fixtures/form.html#L41), and that endpoint returns a 303 to /src/tests/fixtures/one.html resulting in replace action.

It looks like we'll want a new test to validate normal visit redirects result in a new visit proposal with a replace action.

Yes, will do.

this.assert.equal(await this.getSearchParam("greeting"), "Hello from a form")
}

Expand Down
15 changes: 15 additions & 0 deletions src/tests/functional/navigation_tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,21 @@ export class NavigationTests extends TurboDriveTestCase {
this.assert.ok(await this.isScrolledToSelector("#main"), "scrolled to #main")
}

async "test following a redirection"() {
await this.clickSelector('#redirection-link')
await this.nextBody
this.assert.equal(await this.pathname, "/src/tests/fixtures/one.html")
this.assert.equal(await this.visitAction, "replace")
}

async "test clicking the back button after redirection"() {
await this.clickSelector('#redirection-link')
await this.nextBody
await this.goBack()
this.assert.equal(await this.pathname, "/src/tests/fixtures/navigation.html")
this.assert.equal(await this.visitAction, "restore")
}

async "test same-page anchor visits do not trigger visit events"() {
const events = [
"turbo:before-visit",
Expand Down