-
Notifications
You must be signed in to change notification settings - Fork 124
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
Infinite loop of requests after POST request #227
Comments
Hi and thanks for the praise and bringing up this issue and the description of what you're experiencing. <script>
let ajaxify = new Ajaxify();
</script> I don't think that's the issue, because the syntax seems to be alright. Thanks for the picture of your console. Would you have a link to your site? I would be more than happy to try and help you debug this... |
Hi, the alert is run only once and no duplicate init logs are created so it looks like the main problem is with AJAX response handling.
Here are some server responses (Laravel):
So the error occurs when status code is not in |
Hi again. The
...is ignored completely. Really, I think the problem is that the POST somehow attempts to reload the whole library, which causes a sort of "recursion", because the whole logic kicks in by mistake again... Yes, I appreciate, if your site is not online yet, but please take into account as well that offline sites may behave differently... |
Alright, I really can't tell from the information so far.
should only be loaded once on inital page load and never again after that. EDIT: I only saw your PHP script now - will have a look at it! Regarding your PHP test script - why are you using:
within here:
? |
Yes, empty action will make it use the current url. Here it is explicitly set: <?php
if ($_SERVER['REQUEST_METHOD'] === 'POST') {
http_response_code(200); // 200, 404
echo "<p>test</p>";
exit;
}
?>
<html>
<head>
<script src="https://cdn.jsdelivr.net/gh/arvgta/ajaxify@8.2.3/ajaxify.min.js"></script>
<script>
let ajaxify = new Ajaxify({
verbosity: 100,
});
</script>
</head>
<body>
<form method="POST" action="/test.php">
<button>Test</button>
</form>
</body>
</html> No change. |
Exactly. So that was your intention! What is happening now? |
Exactly the same. After each click, more requests are send at once. This is one issue. Another issue is that it tries to load POST request with GET. |
I agree with you that this is weird. Here is the current code within Ajaxify for form handling: class Frms { constructor() {
let fm = 0, divs = 0;
this.a = function (o, p) {
if (!Ay.s.forms || !o) return; //ensure data
if(o === "d") divs = p; //set divs variable
if(o === "a") divs.forEach(div => { //iterate through divs
Array.prototype.filter.call(qa(Ay.s.forms, div), function(e) { //filter forms
let c = e.getAttribute("action");
return(Ay.internal(c && c.length > 0 ? c : Ay.currentURL)); //ensure "action"
}).forEach(frm => { //iterate through forms
frm.addEventListener("submit", q => { //create event listener
fm = q.target; // fetch target
p = _k(); //Serialise data
var g = "get", //assume GET
m = fm.getAttribute("method"); //fetch method attribute
if (m.length > 0 && m.toLowerCase() == "post") g = "post"; //Override with "post"
var h, a = fm.getAttribute("action"); //fetch action attribute
if (a && a.length > 0) h = a; //found -> store
else h = Ay.currentURL; //not found -> select current URL
Ay.Rq("v", q); //validate request
if (g == "get") h = _b(h, p); //GET -> copy URL parameters
else {
Ay.Rq("is", true); //set is POST in request data
Ay.Rq("d", p); //save data in request data
}
Ay.trigger("submit", h); //raise pronto.submit event
Ay.pronto(0, { href: h }); //programmatically change page
q.preventDefault(); //prevent default form action
return(false); //success -> disable default behaviour
});
});
});
};
let _k = () => {
let o = new FormData(fm), n = qs("input[name][type=submit]", fm);
if (n) o.append(n.getAttribute("name"), n.value);
return o;
},
_b = (m, n) => {
let s = "";
if (m.iO("?")) m = m.substring(0, m.iO("?"));
for (var [k, v] of n.entries()) s += `${k}=${encodeURIComponent(v)}&`;
return `${m}?${s.slice(0,-1)}`;
}
}} |
I think POST with status code other than 200-299 is important usecase. If anything goes wrong on the server then response text should be displayed and JS library should not try to send GET request again. Error codes such as 404 and 500 are used by Laravel and other frameworks with custom error pages. In another pjax library https://github.com/MoOx/pjax I handled response in such a way that it displayed error response immediately because it allows to override function: pjax.handleResponse = function(responseText, request, href, options) {
if (responseText && responseText.match("<html")) {
pjax._handleResponse(responseText, request, href, options);
} else {
window.scrollTo(0, 0);
document.write(responseText || request.responseText);
}
} Weird thing is that this is similar to another issue I created in yet another library: PaperStrike/Pjax#342 |
Thanks for your sharp thinking - much appreciated! I'm trying to have a look the last issue in PJAX you mentioned. What does your gut feeling say is the problem in Ajaxify? |
Yes, I can confirm that Ajaxify tries to jump to the URL directly in case of failing of any AJAX request. _lAjax = (hin, pre) => {
var ispost = Ay.Rq("is");
if (pre) rt="p"; else rt="c";
ac = new AbortController(); // set abort controller
rc++; // set active request counter
fetch(hin, {
method: ((ispost) ? "POST" : "GET"),
cache: "default",
mode: "same-origin",
headers: {"X-Requested-With": "XMLHttpRequest"},
body: (ispost) ? Ay.Rq("d") : null,
signal: ac.signal
}).then(r => {
if (!r.ok || !_isHtml(r)) {
if (!pre) {location.href = hin; _cl(); Ay.pronto(0, Ay.currentURL);}
return;
}
rsp = r; // store response
return r.text();
}).then(r => {
_cl(1); // clear only plus variable
if (!r) return; // ensure data
rsp.responseText = r; // store response text
return _cache(hin, r);
}).catch(err => {
if(err.name === "AbortError") return;
try {
Ay.trigger("error", err);
lg("Response text : " + err.message);
return _cache(hin, err.message, err);
} catch (e) {}
}).finally(() => rc--); // reset active request counter
}, I suppose this bit especially: if (!r.ok || !_isHtml(r)) {
if (!pre) {location.href = hin; _cl(); Ay.pronto(0, Ay.currentURL);}
return;
} is not enough error handling? |
Hold on, that code is problematic anyway, because there seem to be two "jumps" happening at once:
|
After having a closer look at your PJAX thread - I feel like simply leaving away the:
...but am afraid that everything will fall over, when changing something that central? |
Double checked against the last jQuery version of the plugin which had been reported to work really well: It only does a:
and nothing else. I suspect the current code in the plain vanilla version does a "double jump"... What would you advise me to prefer? ->
or
The former is safer because it worked in the old jQuery version. |
There is But I think it would be OK to create new config flag, like |
Much appreciated!
The question is, whether the bug does not simply apply to all failed AJAX requests (!) The following code makes sure only non-prefetching requests trigger the code described previously:
My gut feeling says, we need only one of these:
or
I agree - that has been reported elsewhere to be a newly introduced bug in the plain vanilla version. Am calling it a day, too, thanks very much for bringing this up! |
You are welcome. }).then(r => {
if (!r.ok || !_isHtml(r)) {
if (!pre) {location.href = hin; _cl(); Ay.pronto(0, Ay.currentURL);}
return;
}
rsp = r; // store response
return r.text();
On the other hand, Is there a reason that any of them should run if response is not OK or request is not HTML? Why to revisit any URL with GET?
And I think that ajaxify should never repeat any request. Response is received and it should be handled and displayed if it is possible. So it should check if there is any text and just print it. To make it backward compatible, maybe a new config option if (!r.ok || !_isHtml(r)) {
if (revisitOnError) {
if (!pre) {location.href = hin; _cl(); Ay.pronto(0, Ay.currentURL);}
return;
} else {
// handle displaying HTML (ex. 404 Not found styled page) or non HTML such as JSON, etc.
return;
}
}
// handle OK case
rsp = r; // store response
return r.text(); I think it is OK to not store response in history because browser do not store failed responses too. Also response in case of |
First of all, many thanks for your findings, which I all agree with. For a start in here: then(r => {
if (!r.ok || !_isHtml(r)) {
if (!pre) {location.href = hin;/* _cl(); Ay.pronto(0, Ay.currentURL);*/}
return;
}
rsp = r; // store response
return r.text();
} ...I've commented out this code:
...and committed the change to this file for the moment: ...and enabled this file and tested it against 4nf.org briefly - everything still seems to work... Maybe you can see, whether the change has any effect on your system? |
Btw I commented out the entire block and now handlers are not multiplied so next clicks do not generate more and more requests: }).then(r => {
if (!r.ok || !_isHtml(r)) {
// if (!pre) {location.href = hin; _cl(); Ay.pronto(0, Ay.currentURL);}
return;
}
rsp = r; // store response
return r.text();
}).then(r => { Is there a reason why |
Wow, thanks very much for the test. I didn't necessarily think that the change would get rid of the "infinite loop" problem, which surely was bugging lots of users. I will commit this small change asap and create a new version... The other bug, with the wrong
I suppose you mean Ajaxify keeps on attempting the fallback we just debugged, even when:
is specified by the user? That's weird as well, but could we have look at that, when the other fix is committed properly to the repository? |
Yes, if the MIME type is not detected successfully in:
..other MIME types are "jumped" to. I see no other possibility just like the PJAX guys didn't either? |
The bug is caused by ajaxify:
So the problem is that ajaxify repeats the request with GET method. Is there any reason that ajaxify needs to repeat request after fetch was initiated?
This happens with even with What is the |
It is supposed to store, whether a prefetch is being handled
Maybe it really does make sense to add a small check, whether the method is a POST |
But now the condition is not Anyway why ajaxify tries to redirect browser to non-html MIME? JSON is I think this could be configured if to repeat request in certain MIME types, but I think it should try to just display everything if it is possible. |
I agree:
...instead of the current |
Yes, I was just typing this pseudocode!
It could be implemented by checking responseText propertiy or r.text() of the response object. However fetch is weird and in case of some status errors this property is available in catch, see the link: https://stackoverflow.com/questions/36225862/handle-a-500-response-with-the-fetch-api |
I don't think we need a new function:
...because simply returning the text (which is fortunately default at the moment) ought to do the job? The trick with the Yes, this here: is going in the right direction, too... |
And yes, check of method would be good. The main decision is if to handle response by ajaxify or repeat the action with standard browser load with location.href. Here is one idea:
|
Yes exactly, for all these new ideas to work, we would have to tweak Ajaxify to display non-HTML-like content in the case of error. |
Wow - alright! I think we both agree that that would entail quite a bit of additional Ajaxify code? I'm not sure whether we can include an improvement of the dodgy condition:
...as well (these two cases need to be handled separately for sure) |
Yes, that would be very nice. Thank you. |
What do you think? Should we attempt to include a better handling for this condition:
? |
I think in next version just infinite loop could be fixed and next next version two functions could exist:
However _hasTextContent(r) needs to be called in catch block (there is the workaround needed to get response text) so it is more complicated than to modify that condition. |
Thanks! Yes, I agree. I'll release the new version and we'll go from there... |
I have created the new release. |
That condition }).then(r => {
if (!r.ok || !_isHtml(r)) { // hint something is wrong
// store response in any case (location.href = hin would clear)
rsp = r;
r.text().then(text => { // resolve promise right there
if (!text) {
location.href = hin;
return;
}
_cl(1); // clear only plus variable
rsp.responseText = r; // store response text
return _cache(hin, r); // return does nothing probably
});
} else {
rsp = r;
_cl(1); // clear only plus variable
rsp.responseText = r; // store response text
return _cache(hin, r);
}
}).catch(err => { But this is not tested and errors may not be handled. |
Thanks, much appreciated!
I think these two cases should be handled separately in future (the first one first) Thanks for the draft! |
Hi, this library is very nice however I encountered one weird issue. I have a form with POST method which responds with error 500 or 404. When I submit it, many requests are created. Eventually it tries to GET POST-only URL and ends with "405 Method Not Allowed".
See the picture:
I use default config:
Using Firefox on Debian
The text was updated successfully, but these errors were encountered: