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

Version 2 is in beta 🤠 #64

Closed
Sammyjo20 opened this issue Jun 11, 2022 · 71 comments
Closed

Version 2 is in beta 🤠 #64

Sammyjo20 opened this issue Jun 11, 2022 · 71 comments
Assignees
Labels
enhancement New feature or request

Comments

@Sammyjo20
Copy link
Member

Sammyjo20 commented Jun 11, 2022

Hey everyone, thanks so much for all the support that you have given for Saloon. I can't believe it's almost at 500 stars on GitHub and receiving over 150 installs a day. I just wanted to take a moment to thank you all! 😀🙌

That being said, there are some good things coming to Saloon. I'm working on version 2, which will help create a road for the future of Saloon, as well as improving developer experience and making your life easier.

Here is a summary of the changes that are going to happen.

New Flow

Currently, Saloon will run your request through the "Request Manager" which merges all of the headers, config, and everything else from the connector and request into one. With Version 2, I am introducing the "PendingSaloonRequest". Inside of here, this is where all of the logic like merging properties together and running your plugins will happen. This is so the building of a request is entirely separate from the sending of requests. After that, it will be sent to the "Guzzle Sender", and the class will receive the full PendingSaloonRequest with all the final configuration and headers before sending.

I am changing to this new flow because eventually, I want Saloon to be HTTP client agnostic and allow you to use any client you like, so you don't have to use Guzzle if you don't want - and if Guzzle decides to be abandoned, Saloon won't be left in the dark.

Here's the new flow in detail.

image

The only exception to the flow in the image above is that it will not create a PSR-7 request just yet, I am still working that one out.

Other Changes

  • Brand new middleware pipeline so you don't have to rely on Guzzle's handler pipeline
  • Requests don’t go straight to the request manager anymore, they can be built within a PendingSaloonRequest.
  • Better way to send data, by implementing interfaces that are processed by the PendingSaloonRequest.
  • All properties of a request like headers, config, data, handlers etc. Will all be standardised and now in separate buckets like $request→headers()→push().
  • Interfaces to be used a lot more
  • PHP 8.1 minimum
  • Real asynchronous request
  • Request pooling support

Middleware Pipeline

To help move the dependency on Guzzle, Saloon has also implanted its own middleware pipeline for requests and responses. This will replace the old Guzzle Handler support and response interceptors. You will be able to define request pipes and response pipes to modify the request/response before it is sent or given back to the user.

$request = new GetForgeServersRequest;

$request->middleware()
    ->addRequestPipe(function (PendingSaloonRequest $request) {
       //
    })
    ->addRequestPipe(new MyInvokableClass)
     ->addResponsePipe(function (SaloonResponse $response) {
       //
    })

Saloon's middleware pipeline will also be supported for asynchronous requests, so even if you have a pool of requests being sent simultaneously, they can each have their own middleware pipeline, which is something that Guzzle does not support with their existing handler stack logic, since you can only have one handler stack per client.

Middleware pipes can be added anywhere. Inside the request/connector, added by plugins, or even applied right before a request is sent. It will really allow you to tap into Saloon.

External API

Saloon’s external API will still remain the same, like the following:

<?php

$request = new GetForgeServersRequest;
$response = $request->send($mockClient);

$connnector = new ForgeConnector;
$request = $connector->request(new GetForgeServersRequest)->send();
$response = $request->send();

// Or

$connector->send($request);

There will be some additions to the external API, like interacting with request properties

<?php

// Before

$request = new GetForgeServersRequest;
$request->addHeader('X-User-Name', 'Sammy');
$request->addConfig('debug', true);
$config = $request->getConfig(); // Array

// Now

$request->headers()->push('X-User-Name', 'Sammy');
$request->config()->push('debug', true);

$config = $request->config()->all();

// Same with config, handlers, response interceptors, etc.

There will also be some new features, like the ability to set a mock client on the connector or a request, so it doesn’t have to be passed into the send of every request.

<?php

$request = new GetForgeServersRequest;
$request->withMockClient($mockClient);
$request->send(); // Won't need to set it here!

// Even more useful

$connector = new ForgeConnector;
$connector->withMockClient($mockClient);

// All requests using the connector will use the same mock client!

$connector->request($requestA)->send();
$connector->request($requestB)->send();
@Sammyjo20 Sammyjo20 added the enhancement New feature or request label Jun 11, 2022
@Sammyjo20 Sammyjo20 pinned this issue Jun 11, 2022
@juse-less
Copy link
Contributor

Hey, @Sammyjo20.
First off - obviously no stress, just wanted to check in. 🙂

How far would you say you've come on this?
Do you happen to have some documentation and/or notes started, that one could possibly look at?

I've just started looking into both Saloon v2 and Spatie's Laravel Data v2.
I'm not really in a hurry right now (waiting for releases before upgrading), but would like to start experimenting and possibly start making the way for an upgrade, if possible, so I have some new features already using these packages.

As always, thanks for this excellent package, and the superb support. ❤️

@Sammyjo20
Copy link
Member Author

Hey @juse-less,

Thanks for the message - at the moment v2 is in a bit of a funny state, I’ve got a basic request working from start to finish, using the brand new PendingSaloonRequest. That’s pretty much done. The GuzzleSender is about 90% complete and I’m confident that async requests are working properly. However a lot of the tests (pretty much all) are borked at the moment, so I wouldn’t trust all of it.

Overall I’d say I’m about 60% of the way through. The core is in there you can add everything like headers, config, plugins and the brand new middleware pipeline is working very very well.

I’m currently working on the mocking. Since I no longer use Guzzle’s handler stack, I need to build in “early responses” into the middleware/sending pipeline. I’ve got a prototype working.

After I’ve sorted the mocking, it will just be plumbing in all the extra bits like laravel support and OAuth2, but that’s all pretty easy since a lot of it is backwards compatible (except the new headers/config syntax)

I know you are probably anxious about using the async requests, and I can say with confidence that request pooling will work, and you’ll be able to maintain custom config and middleware pipelines for every request. It’s really going to be the dream. I’m also thinking of building pools like this:

$connector = new MyConnector;

$connector->pool([
new RequestOne,
new RequestTwo
])->then()->catch()

and of course you’ll be able to use a generator or maybe an invokable class for the requests.

The way it works is once a request is sent once, we store the request sender on the connector, keeping the current guzzle client open and ready for more requests.

Hope this helps!

P.S I am about to go on holiday for 11 days and I won’t have my laptop, but I can’t wait to get back to coding v2 when I am back!

@Sammyjo20
Copy link
Member Author

As soon as I have a stable-ish API I’ll try to public v2 alphas / betas for you

@juse-less
Copy link
Contributor

juse-less commented Jun 15, 2022

Thanks! That sounds amazing.

The pooling should solve a weird issue we're having with libcurl starting to fail to resolve hosts after ~10 minutes, depending on how quickly we send requests.
The only solution is to restart our Laravel queue workers, or wait for like.. 15-30 minutes before it self-heals (after hundreds of failed Jobs we need to requeue).
I've tried all sorts of settings and configs in Guzzle (even for libcurl directly), to no avail.
I haven't tested PHP's cURL directly, though, but definitely a weird issue.

Nonetheless. Can't wait for it. 😊

Have a great holiday!

@Sammyjo20
Copy link
Member Author

Very strange! Hopefully the pooling will fix the issue. Did pooling fix the problem when you used Guzzle alone?

It’s my pleasure, thank you!

@juse-less
Copy link
Contributor

juse-less commented Jun 16, 2022

I never tried pooling in Guzzle, actually. I just used Laravel's HTTP Client at the time, and quickly tried sending requests with Guzzle directly.
This was also at the time while I was also mid-converting to Saloon to actually use async requests (we used sync with Laravel).
So I did try both sync and async with Guzzle.
As you might've guessed, it became even more apparent when we moved to using async requests, since we send 3-5 requests concurrently.
Our solution for now is to simply restart all Queue workers after 5 minutes, to ensure we don't have a long-running job making it surpass that 10 minute marker.

The problem seems to be that keep-alive requests are being sent (or at least connection handles not being closed correctly), and the handles remain open, causing the whole process (Laravel's Queue worker) to run out of file handles.
I tried disabling keep-alive, enforce HTTP 1.0, etc. But, for some reason, none of seems to do it.
I guess I could just try increasing file handles and see if it can cope with more requests, but.. doesn't feel like an adequate solution.
Since we're doing so many requests to begin with, the correct option would really be to use pooling.
So hopefully it's solved once we move to pooling to keep the connection open, instead of initiating thousands of requests.

I did quickly try both HTTP/2 and HTTP/3, actually, but couldn't get it to work, and didn't have the headroom to test if all different external systems supports either to begin with.

@Sammyjo20
Copy link
Member Author

Sammyjo20 commented Jun 29, 2022

Hey @juse-less - other than the proper support for Async requests, is there anything else in v2 that you need? I may branch out and build request pooling support into v1, with the assumption that Guzzle middleware won't work on the request level.

I'm saying this just because I want to take my time a bit more with v2 and there's going to be quite a lot of work with rewriting the documentation, upgrade guides etc, so I'm wondering if I can help out with the mini feature in v1.

Otherwise, I could suggest that you go back to using your Guzzle approach for the parts of the app that were causing problems because of Saloon's async requests?

@Sammyjo20 Sammyjo20 reopened this Jun 29, 2022
@juse-less
Copy link
Contributor

@Sammyjo20 I think it's ok for now. I appreciate you asked, but I think it's better if you focus on v2. 🙇‍♀️

I apologise for the confusion - what I meant is that I tried with Guzzle as well, but to no avail.
The problem is seemingly that it's opening a new 'file' for each request, and, despite setting libcurl constants (or Guzzle settings) directly in Guzzle, it still didn't fix the underlying issue.
So, ultimately, it would appear that the only option is to support pooling. But I never tried pooling itself, just tried getting Guzzle/libcurl to close the handles in-between async requests.

I think the temporary solution I have in place also helps a bit preventing us from being banned from external systems and we push a lot of requests back and forth.
Other than.. not sending that many requests (and frequently), obviously. 😛

So, that said, I really appreciate you asked and trying to help me with my situation.
But I definitely think it's worth just focusing on v2. 🙂

@Sammyjo20
Copy link
Member Author

Thanks for the clarification @juse-less ! 🙌

I have some other work commitments I also need to spend my time on so v2 might be a bit longer than my initial 4-6 week estimation, but I'll keep updating this when I have more progress to mention. I'm really happy with how v1 works and I think v1 will be fine for most people for the next few months.

If you have any suggestions for v2 please don't hesitate to say!

@Sammyjo20 Sammyjo20 reopened this Jun 29, 2022
@juse-less
Copy link
Contributor

I have a few ideas, that I can write down next week, or so, as I have some work commitments myself (started a tight 2 week sprint today even).
v1 is definitely more than enough. Since I can set Guzzle settings through Saloon already, I could possibly solve my issue in v1 as well, if I can just figure out the Guzzle/libcurl settings to apply.

One thing that directly comes to mind, however, when I quickly tried the parts of v2 last week (but behaves the same in v1), is the usage of interfaces.
I actually register my various connectors, requests, and responses inside the Laravel container.
This is currently not possible, the way that Saloon instantiates the defined class strings from the connector/request/response class string properties.
So, I think there are 2 possibilities.

  1. Have the Laravel Saloon package overwrite certain parts to resolve things from the Laravel service container, or
  2. Easily let us override the sort of.. resolving mechanics, so we can easily call the container with our interface, to resolve the implementation.
    I think this one would be more versatile, as Saloon isn't Laravel-specific. So, letting developers resolve it, they could use other service containers, or other complex logic to resolve them.

It's probably only connectors and responses I'm thinking of, since requests are created directly, but mentioned all 3, just in case.


If you'd like to try something out later on (performance, resource usage, or otherwise), I can definitely try it in our internal tool I'm building.

@bilfeldt
Copy link

bilfeldt commented Aug 6, 2022

Version 1 is awesome - version 2 will be even better I can see 😉 Thanks @Sammyjo20.

The idea about interchangeable HTTP client using PSR-7, PSR-17 and PSR-18 sounds awesome! A good add-on might be adding a compatible driver for the Laravel HTTP Client. I know that under the hood this is using Guzzle, but so many things in the Laravel community is build around this, so you get easy integration with almost anything using that - examples could be:

@Sammyjo20
Copy link
Member Author

Thanks for the message @bilfeldt that means a lot!

I agree it would be amazing to use the existing tools that wrap around Laravel's HTTP client. I will likely release V2 with just Guzzle support initially since that will make it on-par with version one, but the way it's designed, it would be super easy to write a custom adapter for the HTTP client.

Thanks for the feedback and for enlightening me on the idea!

@niladam
Copy link

niladam commented Sep 14, 2022

Hello @Sammyjo20 - and thank you for your package! I've started using it and i'm really happy with it.

I was wondering if you need any help on getting v2 out the door ?

@Sammyjo20
Copy link
Member Author

Hey @niladam

Thank you for the offer but I'm all good, I am getting there but had to stop because work is so busy at the moment and I think I'd fall apart if I worked extra in the evening 😂

I think V2 is definitely going to be here before the end of the year so hang tight!

I will let you know if I need someone to peer review/help me because that would be very helpful.

@Sammyjo20 Sammyjo20 self-assigned this Oct 23, 2022
@Sammyjo20
Copy link
Member Author

Just a little update, I've been starting back up my work for v2! 🙌

I've gone back through my todo list and created a fresh one with all the tasks I want to complete. I will keep the thread updated as progress comes along. My goal is to have it ready before the end of the year, but it may be January when it is released. January would mark Saloon's 1 year anniversary so that would be relevant :D

@Sammyjo20
Copy link
Member Author

Hello again everyone! I wanted to share with you an update on the progress of v2 as I feel it’s approaching the time where I am not introducing any more breaking changes, but I wanted to ask your opinion on the developer experience and how you will feel with a few breaking changes.

Just wanted to note that V2 is NOT ready to be used, even in an alpha state. I need to rewrite a lot of tests and battle-test it. The following is not an upgrade guide either, I am just looking for some feedback before I continue.

Goal

The Goal with v2 is to simplify Saloon’s codebase, make it more future proof and reduce its dependancy on Guzzle allowing it to be used with any HTTP client in the future. V2 also uses a lot more of PSR-7 and full PSR-7 support will likely be released in v3. The gap between v2 and v3 will be much smaller since less breaking changes will be required.

(New, Breaking) PendingSaloonRequest

Previously when you sent a request, Saloon would pass your request instance through the “request manager”. This class would be very closely tied to the HTTP Client (Guzzle) and the entire class was responsible for merging together query parameters, configuration, data and triggering things like authenticators.

Saloon now has a PendingSaloonRequest. This class when created will merge everything together into the one PendingSaloonRequest instance. This prevents the original SaloonRequest class from being polluted with mutations, and also allows Saloon to have a separate class that is built before passing it onto the HTTP Client.

Inside the PendingSaloonRequest’s constructor, it runs various methods to “build up” the pending request.

// PendingSaloonRequest.php / Constructor

$this
  ->registerDefaultMiddleware()
  ->mergeRequestProperties()
  ->mergeData()
  ->bootConnectorAndRequest()
  ->bootPlugins()
  ->authenticateRequest();

This also allows Saloon in the future to convert this class into a PSR-7 request with minimal effort, since it’s already separate.

[Click here to see an example of PendingSaloonRequest](https://github.com/Sammyjo20/Saloon/blob/v2/src/Http/PendingSaloonRequest.php)

(New) Senders

Once a PendingSaloonRequest is created, it will check if a MockResponse has been set. If one hasn’t been set it will pass the PendingSaloonRequest into a “sender”. This sender class is a wrapper around a HTTP Client. The default sender that will ship with Saloon v2 will be the GuzzleSender. Inside of this class, you are required to specify a “sendRequest” method.

The sender instance is created once on the connector and then will be re-used for every request. This allows us to keep the HTTP Client open and allows Saloon to finally support true asynchronous requests. More on that later.

This new sender class will allow Saloon to easily support other HTTP clients in the future without breaking Saloon’s internal logic. This is super exciting because Saloon no longer needs to depend on Guzzle to work. You can customise the sender inside of your own application too if you choose to make your own sender logic.

Another benefit of the senders logic over v1 is that the request and the HTTP client logic is now separated which reduces code complexity.

[Click here to see an example of the GuzzleSender](https://github.com/Sammyjo20/Saloon/blob/v2/src/Http/Senders/GuzzleSender.php)

(Updated, Breaking) Headers, Query Parameters, Config & Data

Saloon v2 also improves the way headers, query parameters, config and data is interacted with. Previously, each bucket of information lived in its own trait. They were inconsistent and sometimes didn’t make too much sense. I’ve now built a standardised “ContentBag” class inspired by Laravel’s MessageBag and ErrorBag classes. These are standardised repository classes that allow you to interact with them each in the same way.

Let’s look at some examples of managing headers.

Old Way

$request = new UserRequest;

$request->addHeader('X-Name', 'Sam');
$request->mergeHeaders([])
$request->setHeaders(['X-Foo' => 'Bar'])
$request->getHeaders(); // array
$request->getHeader('X-Name') // string

New Way

$request = new UserRequest;

$request->headers()->add('X-Name', 'Sam')
$request->headers()->merge([])
$request->headers()->set(['X-Foo' => 'Bar'])
$request->headers()->all()
$request->headers()->get('X-Name') 

The same is true for query, data and config. This is a breaking change, but it massively reduces the complexity of code and makes it easier to test.

Another benefit of using the method access is that default headers will show up! Previously, if you had set an array of default headers in your request and then tried to access the headers by using $request->getHeaders it wouldn’t have shown you the default. Now it will show you the default headers, but it won’t show the default headers on the connector.

The same logic has been shared for query, data and config

$request = new UserRequest;

$request->query()->add()

$request->config()->add()

$request->data()->add()

(Breaking) Data Interfaces Replacing Traits

Previously when you wanted to tell Saloon that a request or connector will have data, you would have to use a plugin like this:

class CreateForgeSiteRequest extends SaloonRequest
{
    use HasJsonBody;

Saloon now has switched to interfaces for data.

class CreateForgeSiteRequest extends SaloonRequest implements SendsJsonBody
{

This is because previously the plugin would add a Guzzle-specific configuration option that adds a JSON, form or multipart body. Saloon will now handle this for you.

(New) Middleware Pipeline

To help move the dependency on Guzzle, Saloon has also implanted its own middleware pipeline for requests and responses. This will replace the old Guzzle Handler support and response interceptors. You will be able to define request middleware and response middleware to modify the request/response before it is sent or given back to the user.

It will support closures and accept a PendingSaloonRequest or an invokable class.

$request = new GetForgeServersRequest;

$request->middleware()
    ->onRequest(function (PendingSaloonRequest $request) {
       //
    })
    ->onRequest(new MyInvokableClass)
    ->onResponse(function (SaloonResponse $response) {
       //
    });

Saloon's middleware pipeline will also be supported for asynchronous requests, so even if you have a pool of requests being sent simultaneously, they can each have their own middleware pipeline, which is something that Guzzle does not support with their existing handler stack logic, since you can only have one handler stack per client.

Middleware pipes can be added anywhere. Inside the request/connector, added by plugins, or even applied right before a request is sent. It will really allow you to tap into Saloon.

It also will across any HTTP Client so in the future if Guzzle is not used, this middleware functionality is Saloon feature.

(Updated, New) Concurrent Requests & Pooling

Saloon’s old design meant that asynchronous requests just didn’t work. This was because every request would create a new Guzzle client, send the request and destruct the object. With Saloon’s new sender logic, Saloon v2 will keep the HTTP Client in memory on the connector. With some caveats.

The following example will not work because a new connector instance will be defined on every request. You must instantiate the connector and use the same connector or use the pool method.

// Will not work

$requestA = new CreateForgeSiteRequest;
$requestB = new CreateForgeSiteRequest;
$requestC = new CreateForgeSiteRequest;

$requestA->sendAsync();
$requestB->sendAsync();
$requestC->sendAsync();

The following examples will work

$conector = new ForgeConnector;

$requestA = new CreateForgeSiteRequest;
$requestB = new CreateForgeSiteRequest;
$requestC = new CreateForgeSiteRequest;

$connector->sendAsync($requestA)
$connector->sendAsync($requestB)
$connector->sendAsync($requestC)
$conector = new ForgeConnector;

$promises = $connector->pool([
	new CreateForgeSiteRequest,
	new CreateForgeSiteRequest,
	new CreateForgeSiteRequest,
]);

Asynchronous requests will return a PromiseInterface instead of SaloonResponse, but it will contain a response inside.

$conector = new ForgeConnector;

$requestA = new CreateForgeSiteRequest;

$promise = $connector->sendAsync($requestA);

$promise
	->then(fn (PsrResponse $response) => ...) // PsrResponse is extension of SaloonResponse
	->catch(fn (Exception) => ...)

(New) PSR Responses

Saloon’s response has also been updated to make it abstract, and you can now make your own response classes that accept different data. This makes it useful if in the future senders need to pass in a different object to responses.

For responses you will receive an instance of PsrResponse. This instance can be created with any class that implement PSR-7’s ResponseInterface class.

The benefit of this is that it makes Saloon almost PSR-7 ready.

(New) Simulated Response

There will be a new SimulatedResponse class that will be sent back if you are using the Saloon cache plugin or a mock response. The API will be the same as the existing SaloonResponse, it will just make it easier to know if a response is real or not.

@juse-less
Copy link
Contributor

Note: I haven't looked through it all just yet, and haven't looked at the progress of the v2 branch.

I have to say, though - I'm liking many of the things I've seen, so far.
The middleware pipeline is gonna be pretty sweet, as I work fairly extensively with first-class callables and invocable classes.


At the top of my head, some things I'd love to see is

  • More strongly typed code. Preferably using generics, where applicable.
    I've had to ignore some stuff in PHPStan, that I believe could be resolved through, say, generics.
    Although, I should've been able to suggest DocBlock changes, or write stubs (this is all new territory for me, though).
  • On top of this, I'd love for Saloon to primarily use interfaces, but supply default implementations (possibly splitting into traits for composability, where applicable).
    That would make it possible to still extend the base class (same as today), but also entirely custom implementations, that could reuse some implementation details through composability.
    The situation with Saloon v1 has made some parts tricky for me, but might be because I've been doing things incorrectly.
    (I have a tendency to think wildly differently than others, and is not directly related to Saloon, but still wanted to mention.)
  • Allow for the SaloonConnector::pool() method to accept arguments along the lines of
    1. $requests - basically either an array, as per the example in your latest message above, a \Generator generating them, or a callable that Saloon has to call in order to retrieve the aforementioned array of requests or \Generator.
      Possibly give the callable the SaloonConnector, or its client as argument (to more easily create async requests for the client/connector).
      As an inspiration, you might look into how Guzzle handles pools, as they support callable|\Generator.
      This is probably a bit extreme and wide typing, though. But one could always wish. 🙂
      array<array-key, PendingSaloonRequest>|\Generator<PendingSaloonRequest>|(callable(): array<array-key, PendingSaloonRequest>|\Generator(PendingSaloonRequest))` $requests
    2. $concurrency - maximum simultaneous requests to 'keep open' at once, in case you need to send many.
      int<0, max> $concurrency

Regarding Promises, it might be good to research around PSR for Promises, but it appears it's kinda died out the past several years.
I know Guzzle is following the Promises/A+ standard, but I've also seen other PHP HTTP libraries follow it.


I haven't been able to make the move into collaborating- or contributing to open source yet, but would give it a shot if you'd want some help (and think it'd be useful).

@Sammyjo20
Copy link
Member Author

Hey @bilfeldt

I'm pleased to announce that Saloon v2 will ship with full support for Laravel's HTTP Client out of the box. By default it will detect that you are using Laravel and it will use send via the HTTP client instead of using Guzzle directly. This is great because you not only get all the power of Guzzle like you did before, but now everything that uses the events for the HTTP client will work through Saloon without any extra configuration.

I think this is going to be huge for Laravel developers!

@bilfeldt
Copy link

bilfeldt commented Nov 4, 2022

I'm pleased to announce that Saloon v2 will ship with full support for Laravel's HTTP Client out of the box. By default it will detect that you are using Laravel and it will use send via the HTTP client instead of using Guzzle directly.

Christmas came early this year 🥳

@Sammyjo20
Copy link
Member Author

Sammyjo20 commented Nov 13, 2022

Just an update everyone - I'm going to be tagging a beta release very soon - I want to fix all the broken tests now that the codebase is stable and settling down. I also want to write the docs for v2 with the first beta so you feel comfortable trying it out. This will come within the next 2-3 weeks, it would be good to have some feedback on

  • The upgrade guide
  • How v2 feels
  • Any new ideas

I'm aiming to have v2 pretty much done before the end of the year. I released v1 of Saloon last year on the 14th January so I think that would be an awesome time to release v2 👀 I'll let you know when it's ready to test!

@Gummibeer
Copy link
Contributor

@Sammyjo20 seems like #101 hasn't found it's way to v2 yet!?

it returns all countries

<html>
<head>
<title>404 Not Found</title>
</head>
<body>
<h1>Not Found</h1>
</body>
</html>

@Gummibeer
Copy link
Contributor

Gummibeer commented Nov 18, 2022

Here's a public upgrade PR: Astrotomic/steam-sdk#1
Besides the missing #101 feature everything seems to work.

Was a bit of "useless" renaming and importing because of changed namespaces, method names and so on.
But was still a pretty fast upgrade - would say around ~15-30min without any upgrade guide but simply running pest until everything was back green. 🙈

And it seems like the whole MockClient thing changed or is broken - I'm on the way to debug it. But the mock client defined on my connector doesn't reach the pending request right now.
Edit: my bad, had to drop the Laravel Saloon wrapper and because of that missed to change the container binding.

@Sammyjo20
Copy link
Member Author

Sammyjo20 commented Jan 17, 2023

Hey @bilfeldt @bnzo I've updated Saloon's "Building SDK" docs with the example of the resource you suggested, thank you for helping! Let me know if there's anything more I should add to this page.

https://docs.saloon.dev/v/2/digging-deepeer/building-sdks

@bilfeldt
Copy link

https://docs.saloon.dev/v/2/digging-deepeer/building-sdks

Good place to put it 👍

@Sammyjo20
Copy link
Member Author

How have people found version two so far? Documentation on my side is coming along really nicely but I don't think we're far off release!

@Sammyjo20
Copy link
Member Author

Hey folks, as I'm writing the last docs I'm making small tweaks to the code and I was just reviewing the DTO conversion logic. This code is on the response ($response->dto()) and I was wondering, should I return null if the response failed, or should I leave it up to the developer to handle an error DTO?

public function dto(): mixed
{
    if ($this->failed()) {
        return null;
    }

    $dataObject = $this->pendingRequest->createDtoFromResponse($this);

    if ($dataObject instanceof WithResponse) {
        $dataObject->setResponse($this);
    }

    return $dataObject;
}

@bilfeldt
Copy link

Hey folks, as I'm writing the last docs I'm making small tweaks to the code and I was just reviewing the DTO conversion logic. This code is on the response ($response->dto()) and I was wondering, should I return null if the response failed, or should I leave it up to the developer to handle an error DTO?

public function dto(): mixed

{

    if ($this->failed()) {

        return null;

    }



    $dataObject = $this->pendingRequest->createDtoFromResponse($this);



    if ($dataObject instanceof WithResponse) {

        $dataObject->setResponse($this);

    }



    return $dataObject;

}

I would say throw a LogicException. Trying to convert an error response to a DTO should not happen 🤷‍♂️

@Sammyjo20
Copy link
Member Author

I would say throw a LogicException.

That's a good way of handling it, but do you think people might have a ServiceError DTO or anything like that?

@bilfeldt
Copy link

I would say throw a LogicException.

That's a good way of handling it, but do you think people might have a ServiceError DTO or anything like that?

Hmmm. It might be that the api errors always comes back in a certain format for a given API. In that case I would implement a custom exception which adds this format (as a dto perhaps). Much like the laravel VilidationException does.

So somewhere it should be possible to take any 4xx error and throw a custom exception.

But IF the user does not throw an exception and does not conditionally check for a success before casting to a DTO, then they are doing something wrong (missing either of those two points), and a LogicalException should be thrown in my opinion 🤷‍♂️

@bnzo
Copy link

bnzo commented Jan 22, 2023

Sometimes you need an exception and sometimes not.
Maybe to mix both worlds, you could implement a default behaviour with ->dto() and another method ->dtoOrFail() or ->dtoOrNull()?

@juse-less
Copy link
Contributor

juse-less commented Jan 22, 2023

I agree with what @bnzo mentions - sometimes you want it, sometimes not.
Both the Response, but also the Request (more of interest) implements the Request::shouldThrowRequestException() method.
Since it's already on the Request, consumers/users can easily override it and define whether or not to throw an Exception.
Then in the Response::dto() method, just check if the Request should throw an Exception.
If true, throw Exception.
If not, return the result of the DTO method on the Request.

public function dto(): mixed
{
    // Should we check failed, or leave it to the user as well?
    if ($this->failed() && $this->pendingRequest->getRequest()->shouldThrowRequestException()) {
        // throw Exception
    }

    $dataObject = $this->pendingRequest->createDtoFromResponse($this);

    if ($dataObject instanceof WithResponse) {
        $dataObject->setResponse($this);
    }

    return $dataObject;
}

@chrisreedio
Copy link

Hello @Sammyjo20 ,

In v1 of the library, with the caching plugin, I could 'overload' the default Cacheable method types to include POST requests so that I wouldn't have to build another cache layer for API tokens.

I see that in the v2 plugin, that is hard coded.

if (! in_array($pendingRequest->getMethod(), [Method::GET, Method::OPTIONS], true)) { return; }

Would you be open to at least reverting the way this works so that we have the option (I used the request constructor to add POST to the array) to cache other method types as well?

@Sammyjo20
Copy link
Member Author

Hey @chrisreedio absolutely, more than happy to add that back in. I'll put that on my list to tackle probably tonight.

How did the upgrade go for you? Was there anything missing on the upgrade guide or could it be improved in any way?

@Sammyjo20
Copy link
Member Author

@chrisreedio This has now been updated in v2.0.0-beta3 of the cache plugin, you can overwrite the getCacheableMethods method now and customise it there :)

@chrisreedio
Copy link

chrisreedio commented Jan 25, 2023

@Sammyjo20

The upgrade went pretty well. Definitely missed having the new docs for the cache plugin but dug into the repo for it and figured it out in maybe 10-15 minutes.

I have a number of packages now that provide SDK's to various third party APIs.
My favorite part of Saloon is how all my 10+ integrations are structured nearly the same.

Reduces mental work when starting a new interaction. All of the 'structure' is already defined and I can focus on the integration itself.

I still need to rework my v1 DTO work into v2 and get that more developed with my calls as I'm mainly still just working with the returned JSON converted into an array/object.

A few of my public usages of Saloon are listed below. Unfortunately some of my mature integrations / usages of Saloon are closed source. The public ones below are all still in an 'experimental' phase but work great so far for my internal proof of concept projects.

Azure Service Bus SDK for Laravel
Azure Data Explorer SDK for Laravel
Azure Data Explorer SDK for PHP - Used by the above Laravel specific package.

I have a 'in project' (not yet packaged) Saloon based integration with VirusTotal, DB-IP, among various others that are still internal that I plan on open sourcing when time permits.

@Sammyjo20
Copy link
Member Author

That's awesome, thanks so much for the feedback @chrisreedio. I'm going to spend a few more evenings this week finishing off the documentation as I feel that's one of the significant parts holding v2 back so far, and then it will be just polishing off the last of v2.

I'm glad the upgrade wasn't too bad.

@bnzo
Copy link

bnzo commented Jan 26, 2023

@chrisreedio I implemented so much times APIs requiring tokens with custom caches because of expiration time... Glad we can deal it with Saloon. Thanks for the repo's references!

@Sammyjo20 waiting for the official release and the full documentation for a final review. I will follow it step by step and see if it flows seamlessly and give you some feed back if it is not :)

@Sammyjo20
Copy link
Member Author

I'm working on a couple of cool features for the next beta 🧑‍🍳

  • The PR will add a dtoOrFail() method which throws a LogicException if the response has failed
  • New hasResponseFailed method on the connector and request so you can change how the response class considers a response as failed.
  • New sendWithRetry method which will allow you to send a request with a specific number of attempts and it will use the $response->failed() method above or handle exceptions and retry the request.

@chrisreedio
Copy link

All three of those will be immediately useful for me @Sammyjo20 . Looking forward to them!

One other bit of feedback, as I commonly find myself building integrations with APIs that have no other known PHP library, further debugging the requests could be quite useful at times.

Pardon if it's already been updated and I haven't noticed but is there any official docs yet on how to swap over to using the Laravel HTTP client so that requests show up in Telescope?

Thanks again!

@Sammyjo20
Copy link
Member Author

Hey @chrisreedio

One other bit of feedback, as I commonly find myself building integrations with APIs that have no other known PHP library, further debugging the requests could be quite useful at times.

Can you elaborate on this one? What kind of debugging functionality would you find useful?

Pardon if it's already been updated and I haven't noticed but is there any official docs yet on how to swap over to using the Laravel HTTP client so that requests show up in Telescope?

I have built the HTTPSender to work with Laravel but I've not written any proper tests with it, so I would like to write some basic sender tests to make sure it works. Once I've written the tests I will write the documentation for it.

@juse-less
Copy link
Contributor

Regarding @chrisreedio's comment on

One other bit of feedback, as I commonly find myself building integrations with APIs that have no other known PHP library, further debugging the requests could be quite useful at times.

Would it make sense to have some 'debug mode' on Saloon, where activating it adds a request/response middleware, calling provided callback(s)?
That way each person can decide if they want to dump information in a log, use spatie/ray, etc.

Something like

// The specific request
Request::debugUsing(callable(Request, Response));
Request::debugRequestUsing(callable(Request));
Request::debugResponseUsing(callable(Response));

// All requests sent through the connector
Connector::debugUsing(callable);
Connector::debugRequestUsing(callable(Request));
Connector::debugResponseUsing(callable(Response));

$api = new MyConnector;
$api->debugUsing(function (Request $request, Response $response): void {
    ray('Request headers', $request->headers());
    ray('Response headers', $response->headers());
});

$api->send(new GetRequest);

// -----
$api->debugRequestUsing(function (Request $request): void {
    ray('Request headers',  $request->headers());
});

// -----
$api->debugResponseUsing(function (Response $response): void {
    ray('Response headers',  $response->headers());
});

If we can easily add middlewares without instantiating the classes, we could make the methods static, and also add to the Response.

Although, it's tricky to debug correctly with middlewares, as we'd need to add the debug middleware at a specific position in the pipeline, depending on what we need to debug.
But maybe it'd be a start?

Having to debug nightmare-APIs myself, this would've significantly helped to see actual payload I send, and actual payload I receive, without needing to add debug stuff in different parts of the code.

@juse-less
Copy link
Contributor

I threw together an (untested) PoC. Something like this:

https://github.com/juse-less/Saloon/commit/bef0422a95a54f197f92b7abcf35b157b1e90a1e

@Sammyjo20
Copy link
Member Author

I think that's a really cool idea. Do you think it's okay enough to log at a PendingRequest level or would you rather debug at the sender level so for example with Guzzle, you'd get the PSR-7 request? The latter would be harder to implement but it's 'lower' down in the chain. Equally PendingRequest should be fine because to debug things like body you can do $pendingRequest->body()->all() etc.

You can now add global middleware with Config::middleware()->onRequest() so your static method could add a global middleware. However I've not figured out a suitable way to unregister the global middleware between tests, so you might need to have an extra Config::stopDebugging() or something at the end of your code or test, then again if it's just for debugging that might be less of an issue.

I'm tagging @Wulfheart here because they mentioned on Twitter about request logging/debugging, so wanted to get all ears on this, may also be worth breaking out into a discussion.

@Sammyjo20
Copy link
Member Author

Just wanted to give a small update on V2 - the docs are almost complete (4 pages left!) We're also working on adding pagination support and Saloon. Next, it'll just be a bit more tidying up and I reckon we can release Saloon over the next 7-14 days! However I will let you all know if any of that changes.

I've been working on the "HttpSender" again @bilfeldt and I am a little worried about how reliable it will be. I had to do quite a lot of funky things to get around Laravel's HTTP client/PendingRequest's code. I'm a little worried about tying it up with v2 of the Laravel package, so I was thinking of branching it out as a separate library where I can make breaking changes if I need to. What are your thoughts? I'm just concerned if people use it, it may not be as reliable as the GuzzleSender.

@bilfeldt
Copy link

bilfeldt commented Feb 8, 2023

Just wanted to give a small update on V2 - the docs are almost complete (4 pages left!) We're also working on adding pagination support and Saloon. Next, it'll just be a bit more tidying up and I reckon we can release Saloon over the next 7-14 days! However I will let you all know if any of that changes.

I've been working on the "HttpSender" again @bilfeldt and I am a little worried about how reliable it will be. I had to do quite a lot of funky things to get around Laravel's HTTP client/PendingRequest's code. I'm a little worried about tying it up with v2 of the Laravel package, so I was thinking of branching it out as a separate library where I can make breaking changes if I need to. What are your thoughts? I'm just concerned if people use it, it may not be as reliable as the GuzzleSender.

@Sammyjo20 Don't ship it as part of this package unless you feel it makes sense - also from a quality perspective :) Curious about what kind of logic you have tied to Guzzle since this did not though seamlessly though? 🤔

@juse-less
Copy link
Contributor

Curious about what kind of logic you have tied to Guzzle since this did not though seamlessly though? 🤔

It's because of how Laravels HTTP client works. I don't remember exactly what is was, at the top of my head, but it's something along the lines of the client resets or ignores some things, making it hard to interact with/extend.
I've seen closed issue(s) about it in Laravels issue tracker, as well.

@chrisreedio
Copy link

chrisreedio commented Feb 9, 2023

@Sammyjo20 I was mainly referring to interfacing with some APIs that are poorly documented so reverting to hit the API and see what I get is sometimes faster than trying to understand what their docs mean or to find the docs are out of date.

This leads into the second part of my comment. When I do the aforementioned process, I usually end up having to log out some combination of the request and/or the response to see what the error is. I also sometimes do Log:: statements with the context containing the request/response. This works just fine but as I usually use Telescope for most of my 'Laravel' debugging/tracing, it would be nice to have it all in one place. Perhaps this workflow is the best but always looking for ways to improve the efficiency of time spent coding. Seems there is never enough time. 🙄

I understand you're running into challenges on this front and IMO v2 shouldn't be held up on this.

Perhaps there is some other mechanism where flag/special method could be set on Saloon so that regular Guzzle requests show up in Telescope?

Perhaps it would just require adding telescope_entries?

I'm not that well versed in adding things to Telescope but could be an alternative if not too in depth.

Thanks again for the great work on this package and releasing it to the world for us to use!

Edit: Perhaps this could be an addon method to the request call? Such as:

$connector = new MyConnector();
$request = new MyRequest('someStringParameter');
$response = $connector->debug(DebugDrivers::TELESCOPE)->send($request);

Perhaps something along these lines? Or could add a debug driver method to the connector/request that would be used if you didn't supply an argument to the debug function? Not sure of the best route here.

Also I had one more thought and maybe there is an easy way to do this but when writing SDK's (as packages) for applications with Saloon, is there a preferred method for the end use application to specify/add a DTO method to the Request class without just extending the request?

@Sammyjo20
Copy link
Member Author

Sammyjo20 commented Feb 9, 2023

Hey @chrisreedio

Thank you for your nice feedback it's always appreciated 🙌

I did actually have a PR for this initially here: saloonphp/laravel-plugin#24. With my original PR, it would have just enabled automatically and start recording all requests into Telescope like before. I can definitely still do that if people would like, but I imagine I would need a way to disable it, especially if people are also using the HttpSender too.

Alternatively, we could look into a debug method in Saloon - drivers could be invokable classes and the Laravel app could provide a "Telescope" driver, but you could also have a "Log" file driver which logs to a filesystem or something like that. This feature can wait until Saloon v2 is released if it's the latter option (which I quite prefer)

What are your thoughts?

Also on this:

Also I had one more thought and maybe there is an easy way to do this but when writing SDK's (as packages) for applications with Saloon, is there a preferred method for the end use application to specify/add a DTO method to the Request class without just extending the request?

I'm not quite sure what you mean sorry? Could you explain a bit more?

@Sammyjo20
Copy link
Member Author

I was wondering if anyone was free to help me try and work out the failing tests on this PR?

saloonphp/laravel-http-sender#2

It's for Laravel's HTTP client sender. I'm really happy with it, except for the test not working. I've left a comment there

@Sammyjo20
Copy link
Member Author

Well... It's official. Saloon v2 is finally out 🎉

Thank you so much everyone for contributing to this conversation and helping build the best project I ever worked on ❤️

There's so many exciting things in the pipeline, first being a full debugging feature. Lots of improvements will be made with the HTTP sender, generics, pagination etc.

See you in the Saloon and thank you again, I'm so grateful for your support!

@Sammyjo20 Sammyjo20 unpinned this issue Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

9 participants