Suggestions for Version 3

[Sammyjo20]

Hey everyone! 👋

I am currently creating a specification for how Saloon version 3 will look and I wanted to create a discussion here so everyone can have a chance to come up with changes/new features for version three.

Some of the new features I’m currently working on are:

• Simplifying some of the code and standardising the PSR-7 request creation a little more by using PSR-17 factories
• Rewriting how paginators work
• Slimming down the code base a little more

Let me know if you have any other ideas!

[nikuscs]

Also i would like to suggest a better way to override the body from the request, there are a lot of poor written apis that often require for example to send the API key as a post param :( currently we are hacking our way with the Pending Request, but could be a better design to interact/modify/merge configs of the connector.

[Sammyjo20]

Hey @nikuscs when you say the body, do you mean the response body from some requests? If so you can overwrite this inside of the PendingRequest as you said, but I can certainly look into a nicer more Saloon-way to overwrite the response body.

[nikuscs]

No, i mean the request body. Usually ( in a perfect world ) you would be pass the the api key / secrets for example into the Connector, and send it on the headers, then all the request sent on that connector would just work, problem is there is a ton of legacy apis that enforece to send api_key on the request body, so the connector in those cases lose a bit of his responsability, and you are forced to either, pass the config into each request, or make a plugin that interacts with pending request and hammer the body all the way down :p, the same happens with urls, say : https://mbway.domain.com and https://creditcard.domain.com.

Ofc these are "edge" cases but the real world is full of them :p hope it was cleae enough.

So in my opinion the interaction with underlying connector should be better and easier to modify

[Sammyjo20]

Ooh I’m not sure if you know @nikuscs but if your request body is either form, multipart or JSON you can actually add the body trait to both the connector and the request and the properties will be merged!

So you could use the “defaultBody” on the connector to add your API keys, and then on a per-request basis have a different request body and it will be merged together.

Merging doesn’t work for string or XML bodies as it’s harder to merge them together.

[nikuscs]

Ooh I’m not sure if you know @nikuscs but if your request body is either form, multipart or JSON you can actually add the body trait to both the connector and the request and the properties will be merged!

So you could use the “defaultBody” on the connector to add your API keys, and then on a per-request basis have a different request body and it will be merged together.

Merging doesn’t work for string or XML bodies as it’s harder to merge them together.

Thanks! was not aware of this indeed, so i assume Json bodies will also work for the merge? How about the URLs ?

[iamniels]

What is the timeline for v3? I have to refactor several SDK's from Saloon v1 to v2 now. Would you suggest to skip v2 and migrate directly to v3?

[Sammyjo20]

Hey @iamniels im hoping to release v3 by the end of July, but you’ll need to upgrade to v2 first. The upgrade to v3 will be fairly minimal and nothing like v1 to v2. Hope the upgrade goes well!

[boryn]

Hi @Sammyjo20!

So you say that migration from v2 to v3 should be quite easy? I gave it a try to the new pagination (not sure if you read #192 (comment)) but actually I can see there are some breaking changes and IMHO all the implemented paginations would have to be written anew? And I would not drop access to $this->originalRequest or not use only lazy collections.

[Sammyjo20]

Hey @boryn I think the pagination is an exception, that's right - you will have to rebuild the paginators but I will be offering some default implementations for out of the box pagination like we have currently. I'll also make sure to add back the original request :)

[Joel-Jensen]

Hi @Sammyjo20, great package!

A few things I've run into that would be great for v3 if no already done:

• NoMockResponseFoundException would be great if the requested URL was part of that exception.
• Have seen other discussions and don't know what the status is but typehint for DTO would be great. Have manage to get it to work sortof by faking it with template docblocks for now tho.
• A ->dto() for the paginator() would be nice to have as a function.

Mocking

• Saloon::fake, Saloon::assertSent and the PendingRequest overall ignore query params. Would be nice to be able to use it like the normal Http without having to build up the url from query params. They also have a bit different syntax ->getMethod()->name, which I understand where it comes from but if it can't be the same underlaying structure, it would be nice to look the same.

Http::assertSent(function ($request) use ($company) {
  return $request->url() == "https://api.test/customers?page=1&limit=1"
      && $request->method() == 'GET';
});

• While on that subject, the doc line "Now when you send requests, they will be sent through Laravel's HTTP client" made me think that Http::preventStrayRequests() and Http::fake() would work, but I guess it's somewhere in between?
• Writing some Saloon::fake() as we speak and all of the first ones wanted the URL without the hostname, then suddently now I'm faced with one that want the full URL.

The overall feeling right now with mocking is that recording was excellent, but all the manual faking feels a bit buggy and I wish I could just use Http::fake instead, especially for my SDK package users, instead of having to write a wiki for what to use instead of Http.

This package is fantastic, keep up the great job! 🌟

[Sammyjo20]

Hey @Joel-Jensen many thanks for taking the time to suggest all this, and for the lovely feedback. Thank you 🙌

NoMockResponseFoundException would be great if the requested URL was part of that exception.

I could make the Request or PendingRequest class available in this exception, which contains the URL?

Have seen other discussions and don't know what the status is but typehint for DTO would be great. Have manage to get it to work sortof by faking it with template docblocks for now tho.

Equally, I'd love to be able to do this. When you say template docblocks do you mean the ones that are inline above a variable definition? Any ideas on how I could improve this, since the dto() method is on the response?

A ->dto() for the paginator() would be nice to have as a function.

This is coming 🙌 but you define how the paginator returns responses, and all you have to do is write a line that calls your method to convert the response into a DTO.

The overall feeling right now with mocking is that recording was excellent, but all the manual faking feels a bit buggy and I wish I could just use Http::fake instead, especially for my SDK package users, instead of having to write a wiki for what to use instead of Http.

I will have a look at the manual mocking and see if I can improve it 👍

[Joel-Jensen]

Hi!

I could make the Request or PendingRequest class available in this exception, which contains the URL?

Realised I could just make a PR, so here it is: #269

Equally, I'd love to be able to do this. When you say template docblocks do you mean the ones that are inline above a variable definition? Any ideas on how I could improve this, since the dto() method is on the response?

My implementation uses the same principle as Gummibear's #120, but I mine is a bit to specific to be applied to a generic package. I also cheat by applying the typehint dockblock inside the resource, and not in the real ->dto() method. I can however say that in VSCode I never seemed to get @usesto work:

/**
 * @template TRequest of \Saloon\Contracts\Request
 *
 * @implements \Saloon\Contracts\Response<TRequest>
 * @uses \Saloon\Traits\Responses\HasResponseHelpers<TRequest>
 */

Instead I had to "fake" the trait functions inside the class, kinda like Laravel Facades. Not as easily maintainable but was better than nothing for me atleast:

/**
 * @template TRequest of \Saloon\Contracts\Request
 *
 * @implements \Saloon\Contracts\Response<TRequest>
 * @method TRequest getRequest();
 */

If that's any help in future work.

[juse-less]

@Joel-Jensen The @uses annotation is used to reference symbols something is using (think: calling a method).

class Example
{
    /**
     * @uses array_map()
     */
    public function mapsArray(array $array): array
    {
        // ...
    }
}

In order to use generics on traits, you'd use @use on the trait use itself.

class Example
{
    /** @use ExampleTrait<MyClass> */
    use ExampleTrait;

    /** @use OtherExampleTrait<MyOtherClass, bool> */
    use OtherExampleTrait;
}

[juse-less]

In terms of generics on DTOs, the overall problem I faced is that generics are not propagated across files, but, primarily, the way Saloon works (to make life simpler and easier for everyone).

Since the majority of users doesn't use custom responses, Saloon cross-references various classes (like Request, PendingRequest, and Response).
These 3, in particular, also cross-reference each other back-and-forth.
This made it quite inconvenient to solve the DTO issue (at least for me).

You have to remember that a PendingRequest has both a Request and Response.
So if we use generics on Request, we also need to 'propagate' that to PendingRequest.

Because the Response is also keeping a reference of both Request and PendingRequest, we also need to add generics for that.

Also keep in mind that things like the Connector, and Dispatcher use the at least one of Request, PendingRequest, and Response (in many cases, more than one).

The closest I got to solving Request, PendingRequest, and Response, without having errors in PHPStan was to deeply nest and repeat the generics, to the point where it just wasn't feasible.
Not from a Saloon perspective, but also from a user perspective.

It's not quite accurate with the code I had at the time, but as an example for the nesting and repeating generics, think:

// As seen below, all 3 types are so dependent on one another, and repeats each other,
//   that it was tremendously hard to figure out.
// The below doesn't work, but gets you an idea of the problem at hand.
// More notably is all instances of TResponse.

/**
 * @template TData
 * @template TResponse of Response<TData, Request<TData, TResponse>, PendingRequest<Request<TData, TResponse>, Response<TData>>, TData>
 */
class Request {}

/**
 * @template TData
 * @template TRequest of Request<TData, TResponse>
 * @template TResponse of Response<PendingRequest<TRequest, Response<TData>>, TData>
 */
class PendingRequest {}

/**
 * @template TData
 * @template TRequest of Request<TData, Response<TData, TRequest, TPendingRequest>>
 * @template TPendingRequest of PendingRequest<TRequest, Response<TData, TRequest, TPendingRequest>>
 */
class Response {}

Even if that had worked, we'd still need to figure out how to solve it for the Connector without making it problematic for users.

Although, as I wrote the example, I might've figured out how to get around Request, PendingRequest, and Response..
It seems to work in PHPStans playground, at least..
But it'd still be a problem with the Connector (f.ex.), where a user would need to define certain things.
At least I think so, but might be solveable, as well.

Technically speaking.. the absolute shortest (but ugly) path to sort DTOs, might be

/**
 * @template TData
 */
interface ResponseContract
{
    /**
     * @return TData
     */
    public function dto(): mixed;
}

/**
 * @template TData
 */
interface RequestContract
{
    /**
     * @param Response<TData> $response
     *
     * @return TData
     */
    public function createDtoFromResponse(Response $response): mixed;
}

/**
 * @template TData
 *
 * @template-implements RequestContract<TData>
 */
class Request implements RequestContract {}

/**
 * @template TData
 *
 * @template-implements ResponseContract<TData>
 */
class Response implements ResponseContract {}

Then just ignore all PHPStan errors we get in Saloon related to that (Connector, Dispatcher, etc, not defining generics).
Since we call Response::dto() to get the actual DTO, that should solve autocompletions.

It's untested, and might need some alteration.. but.. the point is, we could do something like this and ignore all PHPStan errors we can't solve.

[Joel-Jensen]

@juse-less Yeah seems like you have though way harder than me on this. One of these moments you really wish for native type hinted generics in PHP....