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

Clear cache on non-GET form submit #495

Merged
merged 4 commits into from
Jul 19, 2022
Merged

Clear cache on non-GET form submit #495

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
57c3ef3
rebase main
tleish Jun 20, 2022
0356349
rebase cleanup
tleish Jul 18, 2022
c3137ec
fix lint issues
tleish Jul 18, 2022
61ee838
fix lint issues
tleish Jul 19, 2022
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
4 changes: 3 additions & 1 deletion src/core/drive/navigator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,16 @@ export class Navigator {
if (formSubmission == this.formSubmission) {
const responseHTML = await fetchResponse.responseHTML
if (responseHTML) {
if (formSubmission.method != FetchMethod.get) {
const shouldCacheSnapshot = formSubmission.method == FetchMethod.get
if (!shouldCacheSnapshot) {
this.view.clearSnapshotCache()
}

const { statusCode, redirected } = fetchResponse
const action = this.getActionForFormSubmission(formSubmission)
const visitOptions = {
action,
shouldCacheSnapshot,
response: { statusCode, responseHTML, redirected },
}
this.proposeVisit(fetchResponse.location, visitOptions)
Expand Down
17 changes: 15 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 @@ -45,13 +45,15 @@ export type VisitOptions = {
response?: VisitResponse
visitCachedSnapshot(snapshot: Snapshot): void
willRender: boolean
shouldCacheSnapshot: boolean
}

const defaultOptions: VisitOptions = {
action: "advance",
historyChanged: false,
visitCachedSnapshot: () => {},
willRender: true,
shouldCacheSnapshot: true,
}

export type VisitResponse = {
Expand Down Expand Up @@ -85,6 +87,7 @@ export class Visit implements FetchRequestDelegate {
request?: FetchRequest
response?: VisitResponse
scrolled = false
shouldCacheSnapshot = true
snapshotHTML?: string
snapshotCached = false
state = VisitState.initialized
Expand All @@ -99,7 +102,16 @@ export class Visit implements FetchRequestDelegate {
this.location = location
this.restorationIdentifier = restorationIdentifier || uuid()

const { action, historyChanged, referrer, snapshotHTML, response, visitCachedSnapshot, willRender } = {
const {
action,
historyChanged,
referrer,
snapshotHTML,
response,
visitCachedSnapshot,
willRender,
shouldCacheSnapshot,
} = {
...defaultOptions,
...options,
}
Expand All @@ -112,6 +124,7 @@ export class Visit implements FetchRequestDelegate {
this.visitCachedSnapshot = visitCachedSnapshot
this.willRender = willRender
this.scrolled = !willRender
this.shouldCacheSnapshot = shouldCacheSnapshot
}

get adapter() {
Expand Down Expand Up @@ -225,7 +238,7 @@ export class Visit implements FetchRequestDelegate {
if (this.response) {
const { statusCode, responseHTML } = this.response
this.render(async () => {
this.cacheSnapshot()
if (this.shouldCacheSnapshot) this.cacheSnapshot()
if (this.view.renderPromise) await this.view.renderPromise
if (isSuccessful(statusCode) && responseHTML != null) {
await this.view.renderPage(PageSnapshot.fromHTMLString(responseHTML), false, this.willRender, this)
Expand Down
4 changes: 4 additions & 0 deletions src/tests/fixtures/navigation.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ <h1>Navigation</h1>
<input type="hidden" name="path" value="/src/tests/fixtures/one.html">
<button data-turbo-action="replace">Same-origin form[method="post"] button[data-turbo-action="replace"]</button>
</form>
<form id="form-post" method="post" action="/__turbo/redirect">
<input type="hidden" name="path" value="/src/tests/fixtures/one.html">
<input type="submit" id="form-post-submit" value="Submit"/>
</form>
<p><a id="same-origin-false-link" href="/src/tests/fixtures/one.html" data-turbo="false">Same-origin data-turbo=false link</a></p>
<p data-turbo="false"><a id="same-origin-unannotated-link-inside-false-container" href="/src/tests/fixtures/one.html">Same-origin unannotated link inside data-turbo=false container</a></p>
<p data-turbo="false"><a id="same-origin-true-link-inside-false-container" href="/src/tests/fixtures/one.html" data-turbo="true">Same-origin data-turbo=true link inside data-turbo=false container</a></p>
Expand Down
1 change: 0 additions & 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 @@ -146,7 +146,6 @@ test("test standard POST form submission events", async ({ page }) => {

await nextEventNamed(page, "turbo:before-visit")
await nextEventNamed(page, "turbo:visit")
await nextEventNamed(page, "turbo:before-cache")
await nextEventNamed(page, "turbo:before-render")
await nextEventNamed(page, "turbo:render")
await nextEventNamed(page, "turbo:load")
Expand Down
13 changes: 13 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 @@ -136,6 +136,19 @@ test("test following a same-origin POST form button[data-turbo-action=replace]",
assert.equal(await visitAction(page), "replace")
})

test("test following a POST form clears cache", async ({ page }) => {
await page.evaluate(() => {
const cachedElement = document.createElement("some-cached-element")
document.body.appendChild(cachedElement)
})

page.click("#form-post-submit")
await nextBeat // 301 redirect response
Copy link
Contributor

Choose a reason for hiding this comment

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

In the Playwright powered test suite, we need to call the functions that create the Promise instance (as opposed to accessing the Promise instances through the this.nextBeat property).

Similarly, what do you think about prepending an await ahead of the page.click(...) call on the line above?

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 - troubleshooting this right now.

In the Playwright powered test suite, we need to call the functions that create the Promise instance (as opposed to accessing the Promise instances through the this.nextBeat property).

Does this simply mean?:

- await nextBeat
+ await nextBeat()

I'm struggling getting the tests to run in my local MacOS environment.

yarn test:browser --project=chrome                                                                                                                                                        

yarn run v1.22.17
$ playwright test --project=chrome

Running 260 tests using 10 workers


Error: Timed out waiting 120000ms from config.webServer.




  260 skipped
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Any tips for correcting this so I can test locally?

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't experienced that issue before. The local server is still managed by the same code as before. Have you tried yarn run playwright install or yarn run playwright install --with-deps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I ran yarn run playwright install --with-deps first. Will google some more.

Copy link
Contributor

Choose a reason for hiding this comment

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

For what it's worth, I've cloned your branch locally and the tested these changes' test file:

❯ yarn test:browser src/tests/functional/navigation_tests.ts
# ...
  Slow test file: [firefox] › navigation_tests.ts (38s)
  Slow test file: [chrome] › navigation_tests.ts (29s)
  Consider splitting slow test files to speed up parallel execution

  76 passed (41s)
✨  Done in 42.06s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests now report:

  Slow test file: [chrome] › form_submission_tests.ts (39s)
  Slow test file: [chrome] › navigation_tests.ts (24s)
  Consider splitting slow test files to speed up parallel execution

Did this MR impact cause the test to run slower?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, those are already slowwwwww. That output just happened to be part of what I clipped from my terminal.

await nextBeat // 200 response
await page.goBack()
assert.notOk(await hasSelector(page, "some-cached-element"))
})

test("test following a same-origin data-turbo=false link", async ({ page }) => {
page.click("#same-origin-false-link")
await nextBody(page)
Expand Down