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

Fix client-side location not visiting (hard reloading) when returning 409 StatusConflict upon Inertia asset version change. #14

Merged
merged 2 commits into from
Sep 25, 2024

Conversation

the-goodies
Copy link
Contributor

A small refactor to fix client-side not reloading when Inertia has a new asset version. Client side gets a 409 response and does nothing with it - doesn't make a full visit to X-Inertia-Location.
I think a culprit might be that either a response body is not empty or that Content-Type is set to application/json or both of those things.

Now when returning only 2 headers: Vary: X-Inertia and X-Inertia-Location: ... with no other headers and an empty body, it works well and makes a full visit.

Also, I made an early return for this part:

if w2.StatusCode() == http.StatusOK && w2.IsEmpty() {
    i.Back(w, r)
    return
}

@romsar romsar merged commit 345a0cf into romsar:master Sep 25, 2024
1 check passed
@romsar
Copy link
Owner

romsar commented Sep 25, 2024

Now I think that we have to move version check before next.ServeHTTP(w2, r). I guess that if the version is different, we should not get to this part of the code like in the official adapter https://github.com/inertiajs/inertia-laravel/blob/1.x/src/Middleware.php. What do you think?

@romsar
Copy link
Owner

romsar commented Sep 25, 2024

Now I think that we have to move version check before next.ServeHTTP(w2, r). I guess that if the version is different, we should not get to this part of the code like in the official adapter https://github.com/inertiajs/inertia-laravel/blob/1.x/src/Middleware.php. What do you think?

No, I was wrong.
https://github.com/inertiajs/inertia-laravel/blob/1.x/src/Middleware.php#L86
Version check should be after all handlers.

@the-goodies
Copy link
Contributor Author

Yap, version check is after invoking a handler.

I think a culprit might be that either a response body is not empty or that Content-Type is set to application/json or both of those things.

Actually, the real issue was setting X-Inertia: true header (set when calling Inertia.Render).
If we remove it before returning 409, and leave everything else (non-empty body, Content-Type: json, etc,) it still does full reload.

This early return part I made is wrong, because if the request happens to be PUT, PATCH or DELETE, it won't get a correct http.StatusSeeOther response, but instead a default http.StatusFound.

if w2.StatusCode() == http.StatusOK && w2.IsEmpty() {
    i.Back(w, r)
    return
}

So probably we should restore this part (remove early return) and move defer i.copyWrapperResponse(w, w2) before it, like this:

	// Determines what to do when the Inertia asset version has changed.
	// By default, we'll initiate a client-side location visit to force an update.
	//
	// https://inertiajs.com/asset-versioning
	if r.Method == http.MethodGet && inertiaVersionFromRequest(r) != i.version {
		i.Location(w, r, r.URL.RequestURI())
		return
	}

	// Our response writer wrapper does have all needle data! Yuppy!
	//
	// Don't forget to copy all data to the original
	// response writer before end!
	defer i.copyWrapperResponse(w, w2)

	// Determines what to do when an Inertia action returned empty response.
	// By default, we will redirect the user back to where he came from.
	if w2.StatusCode() == http.StatusOK && w2.IsEmpty() {
		i.Back(w2, r)
	}

        // The PUT, PATCH and DELETE requests cannot have the 302 code status.
        // Let's set the status code to the 303 instead.
        //
        // https://inertiajs.com/redirects#303-response-code
	if w2.StatusCode() == http.StatusFound && isSeeOtherRedirectMethod(r.Method) {
		setResponseStatus(w2, http.StatusSeeOther)
	}
})

romsar added a commit that referenced this pull request Sep 25, 2024
Fix empty response redirect issue introduced by #14
@romsar
Copy link
Owner

romsar commented Sep 25, 2024

Thanks! I'll try to find time to write tests for this.

@romsar
Copy link
Owner

romsar commented Sep 27, 2024

Thanks! I'll try to find time to write tests for this.

On current version hard reloading for me is still not working.
I just made some tests, refactoring and fixes that delete Vary and X-Inertia headers in Location method here: #16

@the-goodies
Copy link
Contributor Author

Damn, it really didn't work. My first commit worked (which I tested), because it returned before even invoking a handler.
Later commits moved version check after a handler, but I thought since we are not writing back from w2 to w and early returning, we are safe... But since w2 is using the same underlying header map that w is using, X-Inertia: true header being set on w2 automatically is set on w too.

@romsar
Copy link
Owner

romsar commented Sep 27, 2024

Damn, it really didn't work. My first commit worked (which I tested), because it returned before even invoking a handler. Later commits moved version check after a handler, but I thought since we are not writing back from w2 to w and early returning, we are safe... But since w2 is using the same underlying header map that w is using, X-Inertia: true header being set on w2 automatically is set on w too.

Can you confirm that everything is fine on the latest version now? I've done a couple of tests and everything is working well for me.

@the-goodies
Copy link
Contributor Author

Yes, works well now (v1.3.4) . Tested my previous versions also (v1.3.2 and v.1.3.3) and they don't work.

@romsar
Copy link
Owner

romsar commented Sep 27, 2024

Yes, works well now (v1.3.4) . Tested my previous versions also (v1.3.2 and v.1.3.3) and they don't work.

Okay, thanks for reporting the problem.

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