search

inertiajs / inertia-rails

The Rails adapter for Inertia.js.
https://inertiajs.com
MIT License
529 stars 43 forks source link

Fix modal debugging page #21

Closed ledermann closed 4 years ago

ledermann commented 4 years ago

If an exception occurs while development, Inertia.js displays the error information with debugging tools in a nice modal: https://inertiajs.com/error-handling

Sadly, this does not work with Rails out of the box, because the exception modal looks like this:

before-patch

Why? In Rails, there is a middleware called ActionDispatch::DebugExceptions responsible for rendering exceptions. This middleware checks for xhr? to determine whether the response must be HTML or plain text, see here: https://github.com/rails/rails/blob/v6.0.1/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb#L85

Because an Inertia request is xhr? but should not be considered as an API request, an additional check for the X-Inertia header is needed. I've added a monkey patch to achieve this, now the exception modals looks right:

after-patch

Of course, monkey patching has pitfalls. My patch requires Rails 5.1.0 or later. To support older releases of Rails, the patch needs to be modified.

What do you think? Is patching Rails acceptable for this gem? What is the oldest Rails release Inertia.js should support? Do you see an alternative to patching?

ledermann commented 4 years ago

I cleaned up the monkey patch (now using prepend) and added a similar patch for better_errors, an alternative popular debugging toolkit.

better-errors

BrandonShar commented 4 years ago

This is awesome, I've had to fight with that modal lately, too.

Do you think monkey patching is the only way to accomplish this? I'd prefer to avoid it, but I don't think we need to if it's the best way.

For pre 5.1 is it unusable or would it need a different monkey patch?

ledermann commented 4 years ago

@BrandonShar I don't like monkey patching as well, but I don't see any other way here. In the original source there is a check for xhr? in the middle of a larger method.

For pre Rails 5.1 different patches are required, because Rails has changed implementation over time. I've added more patches down to Rails 4.1, where the plain text rendering was introduced.

bknoles commented 4 years ago

this looks great to me! i also agree that monkey patching is not ideal; however, i've used Better Errors for years without issues and if we are copying their lead I'm comfortable using this solution.

plus, it's very thorough with support for multiple Rails versions

@BrandonShar unless you are strongly opposed, i would say merge it in!

BrandonShar commented 4 years ago

Awesome, I'm good with it. Will merge and cut a new version this week!

Thanks @ledermann !

BrandonShar commented 4 years ago

Hey @ledermann sorry about the delays here, it's been a busy month for us.

I'm going to merge this and cut a new release tomorrow. Since this has some backwards compatibility issues with early rails versions, would you recommend tagging this as a major version bump? Or maybe I misunderstood.

Thanks again for your help!

ledermann commented 4 years ago

In my view there are no compatibility issues with early Rails versions. For Rails 4.1 and later the PR includes individual patches. Older versions (before 4.1) don't need a patch. Overall, merging the PR means that the modal debugging page will work with newer Rails versions without affecting older versions.

IMHO a major version bump is not required.