hotwired / turbo

The speed of a single-page web application without having to write any JavaScript
https://turbo.hotwired.dev
MIT License
6.71k stars 427 forks source link

Not clearing cache when server responds to form submit with text/vnd.turbo-stream.html #554

Open tleish opened 2 years ago

tleish commented 2 years ago

SUMMARY

After submitting a non-GET form and server responds with...

DETAILS

When submitting a non-GET forms, turbo clears it's cache:

class FormSubmission {
  requestSucceededWithResponse(request, response) {
      //...
      this.delegate.formSubmissionSucceededWithResponse(this, response);
    }
  }
}

class Navigator {
  async formSubmissionSucceededWithResponse(formSubmission, fetchResponse) {
    if (formSubmission == this.formSubmission) {
      const responseHTML = await fetchResponse.responseHTML;
      if (responseHTML) {
        if (formSubmission.method != FetchMethod.get) {
          this.view.clearSnapshotCache();
        }
        // ...
      }
    }
  }
}

However, if the server responds with header Content-Type: text/vnd.turbo-stream.html, then the above logic is skipped and the cache is not cleared.

class StreamObserver {
  constructor(delegate) {
    //...
    this.inspectFetchResponse = event => {
      const response = fetchResponseFromEvent(event);
      if (response && fetchResponseIsStream(response)) {
        event.preventDefault();
        this.receiveMessageResponse(response);
      }
    };
    //...
  }
}

class FetchRequest {
  async receive(response) {
    const fetchResponse = new FetchResponse(response);
    const event = dispatch("turbo:before-fetch-response", {
      cancelable: true,
      detail: {
        fetchResponse: fetchResponse
      },
      target: this.target
    });
    if (event.defaultPrevented) {
      this.delegate.requestPreventedHandlingResponse(this, fetchResponse);
    } else if (fetchResponse.succeeded) {
      this.delegate.requestSucceededWithResponse(this, fetchResponse);
    } else {
      this.delegate.requestFailedWithResponse(this, fetchResponse);
    }
    return fetchResponse;
  }
}

When StreamObserver detects fetchResponseIsStream it calls event.preventDefault(). Then in FetchRequest#receive follows the condition event.defaultPrevented which calls FormSubmission#requestPreventedHandlingResponse instead of FormSubmission#requestSucceededWithResponse (mentioned at the beginning of this post). FormSubmission#requestPreventedHandlingResponse does not clear cache.

Is this intentional? For us it feels like a bug.

Testing a possible solution, the following change clears the cache when the server responds with a turbo-stream:

- requestPreventedHandlingResponse(request, response) {
+ async requestPreventedHandlingResponse(request, response) {
    this.result = {
      success: response.succeeded,
      fetchResponse: response
    };
+   const responseHTML = await response.responseHTML;
+   if (responseHTML && this.method != FetchMethod.get) {
+     this.delegate.view.clearSnapshotCache();
+   }
  }

We discovered this issue while using data-turbo-permanent, which cache even with an updated server response.

You can use the following turbo:submit-end event listener workaround:

const FetchMethod = { get: 0 };
const StreamMessage = { contentType: 'text/vnd.turbo-stream.html' };
document.addEventListener('turbo:submit-end', async ({ detail }) => {
  const nonGetFetch = detail.formSubmission.fetchRequest.method !== FetchMethod.get;
  const contentType = detail.fetchResponse.response.headers.get('content-type');
  const fetchResponseIsStream = contentType.startsWith(StreamMessage.contentType);
  const responseHTML = await detail.fetchResponse.responseHTML;
  if (detail.success && nonGetFetch && fetchResponseIsStream && responseHTML) {
    Turbo.clearCache();
  }
});
tleish commented 2 years ago

Related to https://github.com/hotwired/turbo/pull/495, but that MR didn't fix streams. I'm thinking if you want to fix form submission for both turbo-frames and turbo-streams, then adjust the above workaround to:

const FetchMethod = { get: 0 };
document.addEventListener('turbo:submit-end', async ({ detail }) => {
  const nonGetFetch = detail.formSubmission.fetchRequest.method !== FetchMethod.get;
  const responseHTML = await detail.fetchResponse.responseHTML;
  if (detail.success && nonGetFetch && responseHTML) {
    Turbo.clearCache();
  }
});
tleish commented 2 years ago

May also be a duplicate of https://github.com/hotwired/turbo/issues/193

dhh commented 2 years ago

@tleish Seems to me we should indeed invalidate the cache on stream updates as well. But also have to make sure the cache is filled again. Assuming that success/redirect => Turbo Clears Cache => new cache on rendered redirect. So need to match that with streams if we invalidate.

domchristie commented 2 years ago

@tleish with this workaround (https://github.com/hotwired/turbo/issues/554#issuecomment-1078479296) I was seeing errors like:

UnhandledPromiseRejectionWarning: TypeError: undefined is not an object (evaluating 'detail.fetchResponse.responseHTML')

I think this is because a FormSubmissionResult doesn't have a fetchResponse in the failure case. I suppose we could fix this with:

const responseHTML = await detail.fetchResponse?.responseHTML

But I'm wondering why we need to check responseHTML? Is success sufficient?

tleish commented 2 years ago

@domchristie - I honestly do not know the purpose of using responseHTML. I added it when mimicking logic used in used to Navigator#formSubmissionSucceededWithResponse which checks for the existence of responseHTML in order to clear the cache.

see: https://github.com/hotwired/turbo/blob/1ec72ac3236038af50a0010f0821e0664eeac087/src/core/drive/navigator.ts#L86-L105

tleish commented 1 year ago

After creating some tests, I learned that forms with either turbo-frame or turbo-stream responses do not clear cache (updated the title and description).

I feel like the correct implementation of this fix is to move the cache clearing to FormSubmission class, however I'm struggling getting this to work.

src/core/drive/navigator.ts

  async formSubmissionSucceededWithResponse(formSubmission: FormSubmission, fetchResponse: FetchResponse) {
    if (formSubmission == this.formSubmission) {
      const responseHTML = await fetchResponse.responseHTML
      if (responseHTML) {
        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)
      }
    }
  }

src/core/drive/form_submission.ts

  requestPreventedHandlingResponse(request: FetchRequest, response: FetchResponse) {
    this.result = { success: response.succeeded, fetchResponse: response }
+   if(!response.clientError && !response.serverError) {
+     this.requestClearsSnapshotCache(response)
+   }
  }

  requestSucceededWithResponse(request: FetchRequest, response: FetchResponse) {
    if (response.clientError || response.serverError) {
      this.delegate.formSubmissionFailedWithResponse(this, response)
    } else if (this.requestMustRedirect(request) && responseSucceededWithoutRedirect(response)) {
      const error = new Error("Form responses must redirect to another location")
      this.delegate.formSubmissionErrored(this, error)
    } else {
      this.state = FormSubmissionState.receiving
      this.result = { success: true, fetchResponse: response }
+     this.requestClearsSnapshotCache(response)
      this.delegate.formSubmissionSucceededWithResponse(this, response)
    }
  }

+ async requestClearsSnapshotCache(response: FetchResponse) {
+   const responseHTML = await response.responseHTML;
+   if(responseHTML && this.method != FetchMethod.get) {
+     this.delegate.view.clearSnapshotCache();
+   }
+ }

The challenge I'm facing is with this.delegate.view.clearSnapshotCache();. The challenge is getting access to clearSnapshotCache. It works if I edit the javascript directly, but fails when editing in typescript because of interface conflicts with Frame, etc.

I'm less adept to typescript than I am to javascript. Can anyone help with this?

tleish commented 1 year ago

The following does work, but it doesn't feel clean:

  async requestClearsSnapshotCache(response: FetchResponse) {
    const responseHTML = await response.responseHTML;
    if(responseHTML && this.method != FetchMethod.get) {
+     window.Turbo.cache.clear();
    }
  }
domchristie commented 1 year ago

I think my preference would be to duplicate view.clearSnapshotCache() where the response is handled:

  1. standard form POST (Navigator#formSubmissionSucceededWithResponse, already implemented)
  2. frame form POST (FrameController#formSubmissionSucceededWithResponse)
  3. stream POST (Session#receivedMessageFromStream?—this may require updating a method signature to include the response method)

That way FormSubmission is just responsible for submitting the form, and not clearing view caches.