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

Feature | Debug Helper Method #387

Merged
merged 23 commits into from
Mar 20, 2024
Merged

Feature | Debug Helper Method #387

merged 23 commits into from
Mar 20, 2024

Conversation

Sammyjo20
Copy link
Collaborator

@Sammyjo20 Sammyjo20 commented Mar 18, 2024

(Updated description)

This PR introduces a new and exciting way to debug Saloon requests! We previously had the debugRequest and debugResponse methods on the connector/request however they required you to define your own callback to debug with, which was cumbersome.

Let's take a look at an example of dumping out the PSR request's raw request body:

$connector->debugRequest(function (PendingRequest $pendingRequest, MessageInterface $psrRequest) {
    dd($psrRequest->getBody()->getContents());
});

That's a lot of code just for debugging! This PR now makes the callable argument nullable and provides a nice default implementation to quickly debug requests and responses. This is all you have to write now:

$connector->debugRequest()->send($request);

Much cleaner! It will output the raw request in an array format using Symfony's Var Dumper library.

Saloon Request (AlwaysThrowRequest) -> array:6 [
  "connector" => "Saloon\Tests\Fixtures\Connectors\TestConnector"
  "request" => "Saloon\Tests\Fixtures\Requests\AlwaysThrowRequest"
  "method" => "GET"
  "uri" => "https://tests.saloon.dev/api/error"
  "headers" => array:2 [
    "Host" => "tests.saloon.dev"
    "Accept" => "application/json"
  ]
  "body" => ""
]

Responses

But wait, there's more! I have also provided a default implementation for debugging responses. This works in exactly the same way:

$connector->debugResponse()->send($request);
Saloon Response (UserRequest) -> array:3 [
  "status" => 200
  "headers" => []
  "body" => "{"name":"Sam"}"
]

Combined

Most of the time you want to write an even shorter method and get both the request and response at the same time. You can now use a new debug() method.

$connector->debug()->send();
Saloon Request (UserRequest) -> array:6 [
  "connector" => "Saloon\Tests\Fixtures\Connectors\TestConnector"
  "request" => "Saloon\Tests\Fixtures\Requests\UserRequest"
  "method" => "GET"
  "uri" => "https://tests.saloon.dev/api/user"
  "headers" => array:2 [
    "Host" => "tests.saloon.dev"
    "Accept" => "application/json"
  ]
  "body" => ""
]
Saloon Response (UserRequest) -> array:3 [
  "status" => 200
  "headers" => []
  "body" => "{"name":"Sam"}"
]

Note

All of these are available on the request instance too.

Dump & Die

When debugging a common practice is to exit the application after dumping out the useful information. All of the methods now have a "die" argument which can be provided to exit the application.

$connector->debugRequest(die: true);
$connector->debugResponse(die: true);
$connector->debug(die: true);

Todo

  • Write tests
  • Finalise name
  • Decide if we are going to debug the response in a similar way too

@Sammyjo20 Sammyjo20 marked this pull request as ready for review March 18, 2024 23:06
@Sammyjo20 Sammyjo20 changed the title Feature | Debugger Helper Method Feature | Debug Helper Method Mar 18, 2024
@Sammyjo20
Copy link
Collaborator Author

Random idea: Call it yeehaw() and print a big YEEHAW before a request is being sent?

Screenshot 2024-03-18 at 23 28 09

@Sammyjo20
Copy link
Collaborator Author

Now with better styling

Screenshot 2024-03-18 at 23 45 55

@ClaraLeigh
Copy link

Just replying here instead of twitter.

So what I mean by my suggestion was to update the HasDebugging Trait to have two new functions:

trait HasDebugging
{
    public function dump()
    {
        return debug($this);
    }

    public function dd()
    {
        return debug($this, true);
    }
   
    ............

So instead of importing it when used, we could do:

$connector->send($request->dump());

Side note in terms of the code itself. I think laravel Octane has a problem with exit, from memory it causes it to crash octane?
Can't remember the specifics but I know I've run into it a few times when debugging things

@Sammyjo20
Copy link
Collaborator Author

I actually really love that idea. We already have the HasDebugging trait so I can add to it, maybe I just need some heavy docs to make sure people don't get confused and when they call it, expect something to happen?

@ClaraLeigh
Copy link

I think as long as its documented it'll be fine. People always reach for the docs when trying to debug things like this anyway

This however might make it easy enough to remember for next time instead of having to look it up again (like I always do)

@Sammyjo20
Copy link
Collaborator Author

I've added it to the trait, is that what you were thinking? I'm still considering it - I might remove it if I feel it could clash too much with Laravel

@Sammyjo20
Copy link
Collaborator Author

What do you guys think about it also including the response too?

Screenshot 2024-03-19 at 00 51 30

@ClaraLeigh
Copy link

Having both would be perfect and would have saved me a lot of time in the past.

Might be counter intuitive having it on the request instead of the connection but thats minor in comparison to the benefit you get here

@Sammyjo20
Copy link
Collaborator Author

Okay so I've made some more changes - now we have on the connector/request the following:

$connector->debug($die = false); // Outputs request and response
$connector->debugRequest(); // Outputs request (no die)
$connector->debugRequest($closureHandler); // Old functionality
$connector->debugResponse(); // Outputs response (no die)
$connector->debugResponse($closureHandler); // Old functionality

Not even sure if we need the debug() helper if we have this

@ClaraLeigh
Copy link

That looks neat and very usable. Likely easy to remember too

@Sammyjo20
Copy link
Collaborator Author

Thank you for the help with this!

@michaeldyrynda
Copy link

What do you guys think about it also including the response too?

If you debug the connector, include as much as possible for the lifecycle.

If you debug the request, it’s typically because I want to see what is being sent, though I can see a scenario where seeing the response (that would otherwise end up triggering the exception handling), would be helpful

@Sammyjo20
Copy link
Collaborator Author

Hey @ClaraLeigh @michaeldyrynda I have updated the description at the top with the final changes. I have also removed the helper.php file as well as the debug() helper method as I don't think it's needed with the methods described above. Let me know your final thoughts on this! I'll need to write some tests but I'm very pleased with this implementation and I think it's going to be instantly useful.

@boryn
Copy link
Contributor

boryn commented Mar 20, 2024

Hi @Sammyjo20!

Thanks for simplifying debugging!

And what about "entering a debug mode" as using cache or not? For me $connector->debug()->send(); is still too cumbersome ;) If we had something like $this->debugMode we would just with one method like enableDebugMode() display all debugging info, without the need to change the core code ($connector->send();) in different places. Maybe $this->debugMode could be string accepting 'both' / 'request' / 'response'?

PS. Will the old way of debugging be still active? To keep it backwards compatible?

@ash-jc-allen
Copy link

Hey @Sammyjo20! I'm loving the newer version of the debugRequest() method. I think it'd be relatively easy for other developers to discover it too with IDE autocomplete. So I'm all for it 😄

Copy link
Collaborator Author

@Sammyjo20 Sammyjo20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self review

@Sammyjo20
Copy link
Collaborator Author

Hey @boryn thank you for taking a look at this although I don't think I can get it too much simpler than ->debug() 😅! I think if you wanted to debug without adding the method, you could add it via the constructor? I will consider the property or maybe adding a trait.

public function __construct()
{
    $this->debug();
}

Yes the previous functionality will remain 🙌

@Sammyjo20 Sammyjo20 merged commit e669e85 into v3 Mar 20, 2024
14 checks passed
@ClaraLeigh
Copy link

These changes saved me a lot of time this week doing two new api integrations. Very helpful and easy to remember, ty

@Sammyjo20
Copy link
Collaborator Author

Woohoo! That’s great 😀 thank you for letting me know!

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.

None yet

5 participants