Skip to content

Raise turbo:error event on error#560

Open
salomvary wants to merge 4 commits intohotwired:mainfrom
salomvary:add-error-event
Open

Raise turbo:error event on error#560
salomvary wants to merge 4 commits intohotwired:mainfrom
salomvary:add-error-event

Conversation

@salomvary
Copy link
Copy Markdown

This pr continues abandoned work on #357

I am struggling a bit with adding event firing tests as test cases for failure-scenario are missing for most of the affected functionality and I have not found any documentation on he expected behavior either. For example what do we expect if a request in a Turbo frame fails? Seems like the frame is not updated at all. What do we expect if a Drive link fails? Seems like the page gets updated with the error response.

There is still a test failing and I'm not entirely sure what to expect here:

DriveTests - link click network error fires turbo:error event (0.53s)
    AssertionError: expected 'load' to equal 'advance'
    
    A load
    E advance
    
      at DriveTests.test link click network error fires turbo:error event @ src/tests/functional/drive_tests.ts:36:16
      at processTicksAndRejections @ internal/process/task_queues.js:95:5
      at async DriveTests.runTest @ src/tests/helpers/intern_test_case.ts:43:6

And lastly, I'm not entirely sure what the semantics of turbo:error should be. Is this for any fetch error response or exception? Should it be limited to fetch errors only? If that's the case, would turbo:fetch-error be a more appropriate name? I'm personally only interested in fetch error but #357 contains more.

Anyway, feedback welcome.

}
}

export function reportError(error: FetchResponse | unknown) {
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We might want a better name for this as reportError is an existing global: https://developer.mozilla.org/en-US/docs/Web/API/reportError

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can go with processError instead?

@dhh
Copy link
Copy Markdown
Member

dhh commented Jul 16, 2022

Needs a rebase.

@dhh dhh added the needs work label Jul 18, 2022
@nate-at-gusto
Copy link
Copy Markdown

Thanks for resurrecting this @salomvary! I was just looking for this functionality today 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants