arvgta / ajaxify

Ajaxify - The Ajax Plugin
https://4nf.org/
274 stars 124 forks source link

Callback is not called and pronto.error is not fired on 404 #166

Closed tchapi closed 5 years ago

tchapi commented 5 years ago

Hello and thanks for this plugin !

Apparently, the callback should be called when the request hits a 404 (see here), but it does not. Here is my code :

$('#content).ajaxify({
  bodyClasses: true,
  cb: (error) => {
    console.log(error)
  }
})

When I hit a 404, I just get the same behavior as when I hit a 200 : undefined. Is this normal ? I've also tried to subscribe to the pronto.error event as indicated in the doc, but it never fires :

$(window).on('pronto.error', function (event, eventInfo) {
  console.log('error')
})

Am I doing something wrong or am I missing something ?

Thanks a lot ! BR

arvgta commented 5 years ago

Hi,

Glad you like it and thanks for using Ajaxify! The first thing that comes to my mind is that:

$('#content).ajaxify({
  bodyClasses: true,
  cb: (error) => {
    console.log(error)
})

...you're passing a bad parameter to cb (it expects a function - name of an existing function or anonymous) Please fix that and we'll go from there...

Cheers, Arvind

tchapi commented 5 years ago

Well it's a function (it's a lambda function). A normal function yields exactly the same behaviour :

$('#content').ajaxify({
    bodyClasses: true,
    cb: function (error) {
      console.log(error)
    }
  })
arvgta commented 5 years ago

Sorry, I was not aware of "lamda functions".

I have added the following (missing) line in the Ajax-error handling part to this file:

_trigger("error", error);

If we get your use-case working against it, I could craft another release asap

tchapi commented 5 years ago

No problem ;) This line effectively fixes the pronto.error event not being fired, but the callback should be called anyway, no ?

arvgta commented 5 years ago

Yes, exactly, the code for calling back the callback function was there before - very weird...

tchapi commented 5 years ago

(FYI : https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions)

arvgta commented 5 years ago

Yes, thanks! Those "lamda functions" are about as cryptive as it gets... Very elegant!

arvgta commented 5 years ago

Another thing just came to my mind:

Does your 404 page support the content div(s) to be swapped, by any chance? In that case, Ajaxify would not raise an error at all, probably...

EDIT: Not true either - cb should be called in all scenarios...

tchapi commented 5 years ago

Yes, I think cb() should be called in all scenarios, I don't understand why it's not. Can you reproduce on your end ?

arvgta commented 5 years ago

I'm off for the evening - can try tomorrow morning, if you can tell me, what to look for?

tchapi commented 5 years ago

Well It's simple : when navigating to a 404 page via ajaxify, the cb is sometimes not called, and when it is called, the error is undefined

arvgta commented 5 years ago

Just as a warning:

That is what a 404 page looks like on my site - it may be substantially different to yours... (For example: it has a content div)

But if you wish, I'll try my best in the morning...

tchapi commented 5 years ago

A standard 404 page is a page that returns an HTTP code 404 instead of a normal 200 — the error should be raised even if the content of the page has (or hasn't) a content div ... no ? The point here (for me) is to know if the page exists or not when navigating via ajaxify and be able to adapt my behaviour (maybe display an alert but stay on the same page) in case I need to.

arvgta commented 5 years ago

I had the impression, that it may depend on the CMS one uses. What does e.g. Wordpress have a 404.php page for in the theme folder? I believe WP intercepts the 404 and renders the page shown above... (what CMS are you using?)

How do you suggest, should I provoke a "clean / low-level" HTTP 404 in my environment?

tchapi commented 5 years ago

I'm not using any CMS in this case, when hitting a 404, the server just returns an empty response with a 404 code. I think the behavior in your library is fine (treating the response like it is a 200, that is to say updating the #content if there is any), but my concern is that I don't get any error in the callback to indicate that it hit a 404, hence I have no way to know in the callback if the ajax call was a 200 or any other error status.

arvgta commented 5 years ago

Thanks! Exactly, I try to gracefully fill the content div(s) even in the case of an error - glad you approve of that. I have also thought about several things and done some research on the web:

First of all, could you please just quickly see, if the error branch of the central $.ajax() call or more specifically, the new line of code within it:

_trigger("error", error);

...is being reached at all by trying out the handler:

$(window).on('pronto.error', function (event, eventInfo) {
  console.log('error')
})

...once again?

In course of my research, I found out that the proper way to test for a 404 in jQuery is to examine the XHR object returned by $.ajax() - also in case of error:

...more specifically, the desired status is in:

xhr.status

(a numeric value, like 404 or 200)

Fortunately, Ajaxify provides a public method described in:

like this:

...in order to access the internal XHR object

Could you please try out that public method something like this:

in order to assess, whether you can "see" the internal XHR object.

In case that doesn't work, I noticed, that I am using different variables within the error branch of the $.ajax() call:

    lAjax: function (hin, pre) { //execute Ajax load
        var ispost = $.rq("is"); //POST?

        xhr = $.ajax({ //central AJAX load, for both POSTs and GETs
        url: hin, //URL
        type: ispost ? "POST" : "GET", //POST or GET?
        data: ispost ? $.rq("d") : null, //fetch data from $.rq
        success: function(h) { //success -> "h" holds HTML
            if (!h || !_isHtml(xhr)) { //HTML empty or not HTML or XML?
                if (!pre) location.href = hin; //If not a pre-fetch -> jump to URL as an escape
            }

            $.cache1($(_parseHTML(h))); //Clean HTML and load it into cache
            $.pages([hin, $.cache1()]); //Load object into $.pages, too
            plus = 0; //Reset "plus" variable, indicating no pre-fetch has happened

            if (cb) return(cb()); //Call callback if given
        },
        error: function(jqXHR, status, error) {
        // Try to parse response text
            try {
                _trigger("error", error);
                $.log("Response text : " + jqXHR.responseText);
                $.cache1($(_parseHTML(jqXHR.responseText)));
                $.pages([hin, $.cache1()]);
                if(cb) cb(error); 
            } catch (e) {}
        }

...most notably jqXHR, which I reckon should at least be re-assigned to the internal

I'm pretty positive, that we'll get this going somehow. Moreover, this issue probably applies to quite a few users! Thanks again for raising it!

tchapi commented 5 years ago

Hi,

$(window).on('pronto.error', function (event, eventInfo) {
  console.log('error')
})

works well, as expected when you add the trigger. 👍

I've noticed, in the error function of the ajax call, you do :

if(cb) cb(error); 

Shouldn't it be :

if(cb) return (cb(error)); 

... just like in the success handler ? Maybe that's the culprit ..

arvgta commented 5 years ago

Thanks! Much appreciated!

I've inserted the return.

Have you tried out the trick with:

?

Does it work better for you now? Which other changes can we apply?

tchapi commented 5 years ago

Ok, so I've tested a bit :

So I don't know why this callback is behaving like this, I have no clue ... but I will use these tricks in my code for now

arvgta commented 5 years ago

I'm glad at least jQuery.getPage("x") works as expected! Is the callback function being called at all in the error branch of $.ajax()? Is it just the parameter error that is not being passed properly?

tchapi commented 5 years ago

yes it's just the parameter - that is why it's really a mystery !!

arvgta commented 5 years ago

Well, I'm glad the callback is now being called at all in the error branch. Just guessing - maybe the variable name error doesn't go down well with Javascript / jQuery?

I could try giving the parameters in the error branch more specific names? Can you access the error parameter in your new event handler pronto.error?

tchapi commented 5 years ago

I think error is fine. I've used it myself, and it works well in the pronto.error handler. So I'm really clueless here

arvgta commented 5 years ago

Yes, strange indeed... Thanks anyway!

arvgta commented 5 years ago

Just as a recap - it is unclear, whether the missing error value is coming from Ajaxify or the use-case setup... Here is the salient code (of the $.ajax() call):

error: function(jqXHR, status, error) {
    // Try to parse response text
    try {
        _trigger("error", error); //this works
        $.log("Response text : " + jqXHR.responseText);
        $.cache1($(_parseHTML(jqXHR.responseText)));
        $.pages([hin, $.cache1()]);
        if(cb) return cb(error); //error parameter not propagated to the callback function, even though cb is called
    } catch (e) {}
},

...really not leaving much room for failure, as the _trigger() works - including passing the error value -correctly!

Feel free to indicate, should you be experiencing the same...

arvgta commented 5 years ago

Closing because other issues are more impending at the moment and I don't expect anyone else to be able to solve this right now, if "tchapi" can't...