Differing Response Options #123

New Issue

2018-02-15T10:03:15-05:00

zeraphie commented

2018-02-15 10:03:15 -05:00

(Migrated from github.com)

Hey there,

I'm adding PJAX to an application I've been working on in Laravel, but I'm having trouble creating custom responses for when an action is "rejected"

In this particular example, without PJAX added in, the application displays a toast notification when a user tries to delete a resource that is currently being edited by a different person.

At the moment, within the controller I have this response for it

if($request->pjax()){
    return response()->json([
        'flash_message' => $lockoutText
    ]);
}

Which responds with a JSON object with a key of flash (and it correctly returns this response).

However, with PJAX added in, it tries to load the content with the loadUrl method and subsequently with the loadContent method. I'm wondering if a solution to this would be to add something to loadContent at the start which guards against JSON responses and simply returns that as part of the payload for the developer to utilize? Effectively making it into a normal AJAX request? Or is there a way to do this natively with PJAX?

I've got the correct behaviour by adding the following code into the this.request method inside loadUrl while adding a specific trigger for JSON at the same time, though I suspect this is completely unneeded

// If it's a json response, return the response early
if(request.getResponseHeader('content-type') === 'application/json'){
    trigger(document, "pjax:complete pjax:json", request);

    return;
}

Is this something to be considered to add to PJAX?

Hey there, I'm adding PJAX to an application I've been working on in Laravel, but I'm having trouble creating custom responses for when an action is "rejected" In this particular example, without PJAX added in, the application displays a toast notification when a user tries to delete a resource that is currently being edited by a different person. At the moment, within the controller I have this response for it ```php if($request->pjax()){ return response()->json([ 'flash_message' => $lockoutText ]); } ``` Which responds with a JSON object with a key of flash (and it correctly returns this response). However, with PJAX added in, it tries to load the content with the `loadUrl` method and subsequently with the `loadContent` method. I'm wondering if a solution to this would be to add something to `loadContent` at the start which guards against JSON responses and simply returns that as part of the payload for the developer to utilize? Effectively making it into a normal AJAX request? Or is there a way to do this natively with PJAX? I've got the correct behaviour by adding the following code into the `this.request` method inside `loadUrl` while adding a specific trigger for JSON at the same time, though I suspect this is completely unneeded ```javascript // If it's a json response, return the response early if(request.getResponseHeader('content-type') === 'application/json'){ trigger(document, "pjax:complete pjax:json", request); return; } ``` Is this something to be considered to add to PJAX?

BehindTheMath commented

2018-02-15 10:07:14 -05:00

(Migrated from github.com)

Why do you need to use Pjax for this? Why can't you make a simple AJAX request? Pjax isn't meant to be used with Json responses.

zeraphie commented

2018-02-15 10:11:50 -05:00

(Migrated from github.com)

Because if the resource isn't being edited at the time, it should make a normal pjax request to the page instead (to be edited), it is basically decided server-side with which response should be sent

zeraphie commented

2018-02-15 10:20:19 -05:00

(Migrated from github.com)

I forgot to add that also if someone goes to the link without using pjax that it responds with a redirect to the resource homescreen with a flash message of the same text

BehindTheMath commented

2018-02-15 10:24:52 -05:00

(Migrated from github.com)

I don't think this is a common architecture. In my experience, APIs either return server-side rendered HTML or JSON, but not both.

I think it would make more sense for you to extend Pjax and implement your own code to parse the response.

What we can do to make that easier is to move the XHR callback to a separate prototype method of Pjax, to make it easier to override.

@robinnorth What do you think?

I don't think this is a common architecture. In my experience, APIs either return server-side rendered HTML or JSON, but not both. I think it would make more sense for you to extend Pjax and implement your own code to parse the response. What we can do to make that easier is to move the XHR callback to a separate prototype method of Pjax, to make it easier to override. @robinnorth What do you think?

zeraphie commented

2018-02-15 10:33:59 -05:00

(Migrated from github.com)

I'd be happy with a way to get to the request earlier in the call :), like an onrequest function that fires before it does the rest of the request (i.e. above the if (html === false line)

Perhaps something on the lines of:

if(typeof this.onrequest === 'function'){
    this.onrequest(request);

    trigger(document, 'pjax:complete pjax:onrequest', request);

    return;
}

And it's not really working with an API per se

I'd be happy with a way to get to the request earlier in the call :), like an `onrequest` function that fires before it does the rest of the request (i.e. above the `if (html === false` line) Perhaps something on the lines of: ```javascript if(typeof this.onrequest === 'function'){ this.onrequest(request); trigger(document, 'pjax:complete pjax:onrequest', request); return; } ``` And it's not really working with an API per se

robinnorth commented

2018-02-15 11:07:09 -05:00

(Migrated from github.com)

I agree that it's a bit unusual for the same URL route/API endpoint to return a different response type for the same request, dependent on some other condition (in this case, whether a resource is being edited or not). The way that you have it working without PJAX -- a redirect to another route which can then show a flash message -- is more in keeping with RESTful API best practices, as you're making use of HTTP status codes correctly.

PJAX already handles requests based on HTTP status and considers anything that returns a code other than 200 (OK) an error, which results in the pjax:error event being triggered on the document element. @BehindTheMath, I wonder if actually we could just pass the request object to the error event to allow applications using PJAX to then use the request object to handle the error condition however they see fit. In @zeraphie's case, that would be to get the request.responseText, parse it as JSON and do something with it to show the user a message.

I agree that it's a bit unusual for the same URL route/API endpoint to return a different response type for the same request, dependent on some other condition (in this case, whether a resource is being edited or not). The way that you have it working without PJAX -- a redirect to another route which can then show a flash message -- is more in keeping with RESTful API best practices, as you're making use of HTTP status codes correctly. PJAX already handles requests based on HTTP status and considers anything that returns a code other than 200 (OK) an error, which results in the `pjax:error` event being triggered on the document element. @BehindTheMath, I wonder if actually we could just pass the request object to the error event to allow applications using PJAX to then use the request object to handle the error condition however they see fit. In @zeraphie's case, that would be to get the `request.responseText`, parse it as JSON and do something with it to show the user a message.

BehindTheMath commented

2018-02-15 16:55:28 -05:00

(Migrated from github.com)

I wonder if actually we could just pass the request object to the error event to allow applications using PJAX to then use the request object to handle the error condition however they see fit. In @zeraphie's case, that would be to get the request.responseText, parse it as JSON and do something with it to show the user a message.

That's a good idea. Regardless whether it solves this issue, it would be a good idea to give the user access to the response so they can decide what to do in event of unexpected data.

>I wonder if actually we could just pass the request object to the error event to allow applications using PJAX to then use the request object to handle the error condition however they see fit. In @zeraphie's case, that would be to get the request.responseText, parse it as JSON and do something with it to show the user a message. That's a good idea. Regardless whether it solves this issue, it would be a good idea to give the user access to the response so they can decide what to do in event of unexpected data.

👍 2

zeraphie commented

2018-02-16 03:45:14 -05:00

(Migrated from github.com)

That's pretty much what I was looking for :)

👍 2

BehindTheMath commented

2018-02-27 13:29:10 -05:00

(Migrated from github.com)

However, with PJAX added in, it tries to load the content with the loadUrl method and subsequently with the loadContent method.

@zeraphie Where exactly does the error occur? Is it caught by this catch block?

>However, with PJAX added in, it tries to load the content with the loadUrl method and subsequently with the loadContent method. @zeraphie Where exactly does the error occur? Is it caught by [this `catch` block](https://github.com/MoOx/pjax/blob/0c7af354fdbd8a5bd287150ba12ae8e7b10208eb/index.js#L202)?

zeraphie commented

2018-02-27 17:11:58 -05:00

(Migrated from github.com)

Ahhhh I think it was a thrown error but I'd need to be at work to double check

BehindTheMath commented

2018-02-27 17:20:51 -05:00

(Migrated from github.com)

Ok. I'm just trying to determine where to put the error handling code.

robinnorth commented

2018-02-28 11:21:28 -05:00

(Migrated from github.com)

If we implement my suggestion of relying on the user making use of HTTP status codes (e.g. returning a 302 redirect in @zeraphie's case), then this block should trigger the pjax:error event that we'd want to pass the request object to.

If we implement my suggestion of relying on the user making use of HTTP status codes (e.g. returning a 302 redirect in @zeraphie's case), then [this block](https://github.com/MoOx/pjax/blob/a72880d205c904b94bca3b9724336ea46a4890df/index.js#L156-L160) should trigger the `pjax:error` event that we'd want to pass the request object to.

BehindTheMath commented

2018-02-28 11:29:30 -05:00

(Migrated from github.com)

If we implement my suggestion of relying on the user making use of HTTP status codes (e.g. returning a 302 redirect in @zeraphie's case), then this block should trigger the pjax:error event that we'd want to pass the request object to.

True. However, if it was a 200 response, but the body was malformed for some reason, it would not be caught there. It might be caught here, but I haven't tested that yet. Additionally, that doesn't trigger a pjax:error event.

>If we implement my suggestion of relying on the user making use of HTTP status codes (e.g. returning a 302 redirect in @zeraphie's case), then this block should trigger the pjax:error event that we'd want to pass the request object to. True. However, if it was a 200 response, but the body was malformed for some reason, it would not be caught there. It might be caught [here](https://github.com/MoOx/pjax/blob/0c7af354fdbd8a5bd287150ba12ae8e7b10208eb/index.js#L202), but I haven't tested that yet. Additionally, that doesn't trigger a `pjax:error` event.

BehindTheMath commented

2018-03-06 13:43:20 -05:00

(Migrated from github.com)

@zeraphie Were you able to check?

zeraphie commented

2018-03-07 10:33:58 -05:00

(Migrated from github.com)

Hey!

Sorry, I've been swamped with work lately but should be able to check shortly!

Hey! Sorry, I've been swamped with work lately but should be able to check shortly!

zeraphie commented

2018-03-09 05:33:51 -05:00

(Migrated from github.com)

Yeah, I can confirm it is caught there

👍 1

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: iLoveElysia/pjax#123