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

Make automatic redirect opt-in instead of opt-out #410

Closed

Conversation

RobertBoes
Copy link
Contributor

@RobertBoes RobertBoes commented Jun 2, 2022

This PR makes the automatic redirect back from an empty response opt-in instead of the current opt-out.

The reasoning behind this: I just created a fresh Inertia install and started building with TDD. After writing a test I ran it and immediately I was confused, one of the very first failures was an unexpected status code. Instead of a 200 I received a 302. I searched for quite some time, thought it might've been an incorrect middleware or something else really funky going on. Looked through Inertia's release notes and didn't see anything obvious that could cause it (I later noticed the releases on the website aren't maintained). After digging for a while I noticed #350 introduced a redirect back when an empty response is returned. Enabling this by default is extremely confusing imo, since a default Laravel app will never do this, but instead it creates an empty 200 response.

@reinink
Copy link
Member

reinink commented Jun 2, 2022

Hey @RobertBoes! 👋

So while I get your point, and this was a concern I had at first when adding this feature, I actually think it's fine as it is, since this behavior only happens on Inertia requests/responses. If it happened for all requests, then I think I'd agree with you.

That said, I think it should definitely be documented better. I am going to add this to my list of things to add to the website.

Hope that makes sense, and thanks either way for the contribution 👍

@reinink reinink closed this Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants