hotwired / turbo

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

drive: HTTP status 404 behaviour is buggy and unpredictable #712

Open bep opened 2 years ago

bep commented 2 years ago

This has been a topic before, e.g. with @denydias 's comment in https://github.com/hotwired/turbo/issues/188#issuecomment-1101973056

That issue is closed, but I have tested this with the Turbo main branch as of yesterday (I notice there have since been some commits).

I see there have been some work on this with the turbo:frame-missing for Turbo Frames, but for Turbo Drive this still leads to a faulty and unpredictable state.

I have a patched version that simply add the 404 to the OK list here:

https://github.com/hotwired/turbo/blob/d81e24db77da05f53c6ab3c66e2620574c9e08c9/src/core/drive/visit.ts#L490

function isSuccessful(statusCode: number) {
  if (statusCode == 404) {
    return true
  }
  return statusCode >= 200 && statusCode < 300
}

I would say that from a rendering perspective (which is Turbo's domain), a 404 is (almost always?) a correct thing to do.

dhh commented 2 years ago

cc @seanpdoyle

seanpdoyle commented 2 years ago

@bep thank you for opening this issue. Are you able to reproduce this behavior with https://github.com/hotwired/turbo/releases/tag/v7.2.0-rc.2?

bep commented 2 years ago

@seanpdoyle I have tested my app with both v7.2.0-rc.2 and v7.2.0-rc.1. I'm using Turbo with an AlpineJS app, an what I see on 404 errors are some uncaught JS errors indicating a somewhat corrupt state:

image image

I think this issue is about 2 things:

  1. Whether 404 should be considered an error situation (from a rendering perspective). In my experience, the answer is almost always no (render a 404 page with a linke to the home page or something).
  2. For real errors, Turbo users should be given a way to somehow recover.

This GItHub issue is mostly about 1, and I would be a happy camper if isSuccessful would consider 404 to be a success (or if this could somehow be configured).