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.

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

Raise turbo:error event on error #560

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

salomvary
Copy link

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.

@@ -82,3 +84,8 @@ export function clearBusyState(...elements: Element[]) {
element.removeAttribute("aria-busy")
}
}

export function reportError(error: FetchResponse | unknown) {
Copy link
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
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
Member

dhh commented Jul 16, 2022

Needs a rebase.

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

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