Clear cache on non-GET form submit

[tleish]

Closes #193

When handling a form submission, turbo clears cache but then re-creates a cache of the current page before navigating to the new page.

  async formSubmissionSucceededWithResponse(formSubmission: FormSubmission, fetchResponse: FetchResponse) {
    if (formSubmission == this.formSubmission) {
      ///...
      if (responseHTML) {
        if (formSubmission.method != FetchMethod.get) {
          this.view.clearSnapshotCache() //<= clear cache
        }

        ///...
        this.proposeVisit(fetchResponse.location, visitOptions) //<= recreates cache
      }
    }
  }

[tleish]

@seanpdoyle - one question with this MR, it only covers visits (not turbo-frame or turbo-stream). In other words:

• Visit Non-Get Form Submission clears cache
• Turbo-frame Non-Get Form Submission DOES NOT clear cache (should it clear?)
• Turbo-stream Non-Get Form Submission DOES NOT clear cache (should it clear?)

FYI: I currently have a situation using turbo-stream where I expected it to clear the cache, but it did not.

[seanpdoyle]

I cloned this locally, rolled back the implementation changes, and wasn't able to get the new test to fail.

I wonder if there's another way to reproduce the behavior you're experiencing in your application within the Turbo test suite.

[seanpdoyle]

Is this assertion omitted because we're no longer firing the turbo:before-cache event from submissions?

That's a breaking change to me, since the documentation encourages applications to listen for turbo:before-cache events to deconstruct any DOM extensions and plugins.

Could we continue to fire the event, but handle the snapshot caching internally?

I'm imagining a less intrusive diff that re-uses some of the already existent VisitOptions keys.

It could extend the visitCachedSnapshot(Snapshot) callback to also accept the SnapshotCache instance, then clear the cache after a destructive form submission:

diff --git a/src/core/drive/navigator.ts b/src/core/drive/navigator.ts
index 59b2ad0..8fdccbb 100644
--- a/src/core/drive/navigator.ts
+++ b/src/core/drive/navigator.ts
@@ -6,6 +6,8 @@ import { expandURL, getAnchor, getRequestURL, Locatable, locationIsVisitable } f
 import { getAttribute } from "../../util"
 import { Visit, VisitDelegate, VisitOptions } from "./visit"
 import { PageSnapshot } from "./page_snapshot"
+import { Snapshot } from "../snapshot"
+import { SnapshotCache } from "./snapshot_cache"
 
 export type NavigatorDelegate = VisitDelegate & {
   allowsVisitingLocationWithAction(location: URL, action?: Action): boolean
@@ -85,13 +87,15 @@ export class Navigator {
     if (formSubmission == this.formSubmission) {
       const responseHTML = await fetchResponse.responseHTML
       if (responseHTML) {
+        let visitCachedSnapshot = (snapshot: Snapshot, snapshotCache: SnapshotCache) => {}
         if (formSubmission.method != FetchMethod.get) {
           this.view.clearSnapshotCache()
+          visitCachedSnapshot = (snapshot, snapshotCache) => snapshotCache.clear()
         }
 
         const { statusCode, redirected } = fetchResponse
         const action = this.getActionForFormSubmission(formSubmission)
-        const visitOptions = { action, response: { statusCode, responseHTML, redirected } }
+        const visitOptions = { action, response: { statusCode, responseHTML, redirected }, visitCachedSnapshot }
         this.proposeVisit(fetchResponse.location, visitOptions)
       }
     }
diff --git a/src/core/drive/visit.ts b/src/core/drive/visit.ts
index 9e900b8..3bf4dc2 100644
--- a/src/core/drive/visit.ts
+++ b/src/core/drive/visit.ts
@@ -4,6 +4,7 @@ import { FetchResponse } from "../../http/fetch_response"
 import { History } from "./history"
 import { getAnchor } from "../url"
 import { Snapshot } from "../snapshot"
+import { SnapshotCache } from "./snapshot_cache"
 import { PageSnapshot } from "./page_snapshot"
 import { Action } from "../types"
 import { uuid } from "../../util"
@@ -43,7 +44,7 @@ export type VisitOptions = {
   referrer?: URL,
   snapshotHTML?: string,
   response?: VisitResponse
-  visitCachedSnapshot(snapshot: Snapshot): void
+  visitCachedSnapshot(snapshot: Snapshot, snapshotCache: SnapshotCache): void
   willRender: boolean
 }
 
@@ -73,7 +74,7 @@ export class Visit implements FetchRequestDelegate {
   readonly action: Action
   readonly referrer?: URL
   readonly timingMetrics: TimingMetrics = {}
-  readonly visitCachedSnapshot: (snapshot: Snapshot) => void
+  readonly visitCachedSnapshot: (snapshot: Snapshot, snapshotCache: SnapshotCache) => void
   readonly willRender: boolean
 
   followedRedirect = false
@@ -396,7 +397,7 @@ export class Visit implements FetchRequestDelegate {
 
   cacheSnapshot() {
     if (!this.snapshotCached) {
-      this.view.cacheSnapshot().then(snapshot => snapshot && this.visitCachedSnapshot(snapshot))
+      this.view.cacheSnapshot().then(snapshot => snapshot && this.visitCachedSnapshot(snapshot, this.view.snapshotCache))
       this.snapshotCached = true
     }
   }

What do you think?

[tleish]

Is this assertion omitted because we're no longer firing the turbo:before-cache event from submissions?

That's a breaking change to me, since the documentation encourages applications to listen for turbo:before-cache events to deconstruct any DOM extensions and plugins.

Correct. Seems odd to fire a turbo:before-cache event when it's not going to cache. If the page includes <meta name="turbo-cache-control" content="no-cache">, then turbo does not send a turbo:before-cache event. Would this then be reported as a bug if turbo:before-cache is fired, but not actually cached?

[dhh]

Seems correct to me that we wouldn't fire turbo:before-cache if we indeed no longer cache the page on form submissions? CC @seanpdoyle

[tleish]

I cloned this locally, rolled back the implementation changes, and wasn't able to get the new test to fail.

Odd. It failed for me locally and in the Github CI pipeline before adding the implementation.

[dhh]

Can we rebase and try again?

[tleish]

@dhh - I can rebase, but there's still an outstanding conversation with @seanpdoyle that needs a decisions.

see: https://github.com/hotwired/turbo/pull/495/files#r765369966

[dhh]

I think we're good to not trigger before:cache in this case, when no caching is happening. We have other lifecycle hooks to use for general transitions. I think we're good to rebase this.

[seanpdoyle]

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?

[tleish]

@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?

[seanpdoyle]

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?

[tleish]

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

[seanpdoyle]

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.

[tleish]

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?

[seanpdoyle]

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