Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Nice job! And FR: Swappable PSR-7-compliant HTTP client library #7

Closed
simonhamp opened this issue Jan 20, 2022 · 4 comments
Closed
Assignees
Labels
enhancement New feature or request

Comments

@simonhamp
Copy link

I came here to say great work on putting this together. It looks good and it's really nicely organised. I've only been reading code and haven't had a chance to try it out yet, but I feel like this could become a great package to make integrating with lots of APIs easier!

One thing I picked up on pretty quickly though is how dependent it is on Guzzle (I love Guzzle btw, so it's fine for me). I know it's early days for this package and I'm sure (/hope) you have plans in mind to make this not such a hard dependency as it feels quite useful for such a fundamental (and PSR-compliant) library to be easily replaceable.

To that end though, one thing that I spotted that will kind of make this harder is that the request config (and possibly other structures) is exposed directly to Guzzle: your SaloonRequest class (or more correctly, the CollectsConfig trait) gathers it all up and essentially passes it Guzzle unfiltered in your RequestManager.

It feels like a good goal would be to abstract this away so that Guzzle could be swapped out as needed.

Eventually you'd end up with a standard set of config options that work across HTTP client libraries. Even if your internal structure matches Guzzle's for convenience (and it may stay that way for a long time), you could introduce a transform layer between your structure and the HTTP client being used so that devs can use a consistent interface across libraries.

You and users of this package would be less susceptible to changes on Guzzle's side, for example if they suddenly removed an option, you could help maintain backwards compatibility for folks.

You could then also remove Guzzle as a dependency, which means fewer potential conflicts for people too.

@Sammyjo20
Copy link
Collaborator

Sammyjo20 commented Jan 21, 2022

Hi @simonhamp, thank you for your comments, I really appreciate it!

I'll start off with saying I really like this idea. I don't know a huge amount about PSR-7 (I think is the right PSR) but I do know that Guzzle follows it quite well. I'd like in the future to be able to make swappable clients for Saloon so you can easily build your own Saloon clients/adapters that it uses under the hood.

The main request flow for Saloon is as follows:

  1. Request is created and sent method is called
  2. Request goes to request manager
  3. RequestManager builds up and merges headers, config, interceptors and handlers defined on both the request and the connector
  4. RequestManager converts it to a Guzzle response
  5. Guzzle sends the request
  6. Saloon receives the response
  7. Saloon sends you the response

I think Saloon could replace the RequestManager with a SaloonClient, with the default being GuzzleClient. Inside of the client, it would be responsible for building up the request, but it would have access to:

  • All the merged headers, config, handlers and interceptors
  • The request URL
  • The ability to create a Saloon response

That way, you would be able to create any client you like and apply the headers/config/handlers in whichever way the client supports it. If a particular client didn't support middleware/handlers like Guzzle does, maybe you can specify that in the client so Saloon would know to disable that particular feature.

What are your thoughts?

I feel that this would take Saloon to the next level, from being a Guzzle wrapper to being the "Flysystem" of HTTP integrations. As exciting as this is, I'm happy to leave it dependant on Guzzle for the next few months at least, because it's really solid so far, and then as it matures I hope to use this client pattern. I've got a couple of things I want to build in Saloon first like mocking and caching. Once I've built that - I'll say Saloon is ready for version 1.0, after that I'll look into making it pluggable.

@Sammyjo20 Sammyjo20 added the enhancement New feature or request label Jan 21, 2022
@Sammyjo20 Sammyjo20 self-assigned this Jan 21, 2022
@simonhamp
Copy link
Author

Totally agree! Only build it if/when it's needed.

Regardless, I'm looking forward to using this package soon

@Sammyjo20
Copy link
Collaborator

Sounds great! I’ll happily keep this issue open for now so others can know it’s on my mind.

Hope you have fun with it!

@Sammyjo20
Copy link
Collaborator

I will convert this into a discussion to move it away from the issues page 🚀

@saloonphp saloonphp locked and limited conversation to collaborators Apr 13, 2022
@Sammyjo20 Sammyjo20 converted this issue into discussion #50 Apr 13, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants