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

PSR-ify the CourierClient 🔌 #29

Merged
merged 12 commits into from
Feb 3, 2022
Merged

PSR-ify the CourierClient 🔌 #29

merged 12 commits into from
Feb 3, 2022

Conversation

slashequip
Copy link
Contributor

@slashequip slashequip commented Jan 28, 2022

Description of the change

#7 was raised as a need to switch the HTTP client to something more well know.

A better practice now in PHP, following PSR standards, is to allow the consumer of the package to decide which HTTP client they would like to use and work with abstractions in your package.

This PR introduces the above abstractions and uses discovery to locate consumers' client and use that according to the spec.

This means we no longer need to provide a concrete HTTP client along side this package.

This PR is considered a breaking change, consumers who pull in this new version will need to ensure they also require a concrete implementation. All major HTTP clients follow PSR now and can be found on the list of clients that provide this implementation, as well as adapters if needed.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Related issues

Closes #7

Checklists

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests
  • All tests related to the changed code pass in development

Screenshot 2022-01-28 at 19 17 53
Screenshot 2022-01-28 at 19 18 08
Screenshot 2022-01-28 at 19 18 30
Screenshot 2022-01-28 at 19 18 46

Code review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
  • "Ready for review" label attached to the PR and reviewers mentioned in a comment
  • Changes have been reviewed by at least one other engineer
  • Issue from task tracker has a link to this pull request

@slashequip
Copy link
Contributor Author

@tk26 I'm just going to run some of this code locally, with a concrete client installed to ensure things work as expected - to be double sure! I'll ping you again when I'm happy this can be merged.

@tk26
Copy link
Contributor

tk26 commented Jan 28, 2022

sounds great! thanks @slashequip ❤️

Although MessageFactoryDiscovery feels better to create a request it's actually deprecated.
@slashequip
Copy link
Contributor Author

Ok I'm happy now, made some final adjustments to use non-deprecated discovery classes.

This PR now works out of the box with a new Laravel install (which comes with Guzzle7 installed) - no fuss, no frills, just works. 🎉

@slashequip slashequip changed the title [Draft] PSR-ify the CourierClient 🔌 PSR-ify the CourierClient 🔌 Jan 28, 2022
@tk26
Copy link
Contributor

tk26 commented Feb 2, 2022

@slashequip changes look super cool to me! Just had a quick (noob alert) question: For folks having laravel in their stack, the package would work out of the box; I was curios whats the setup when someone doesn't have laravel installed? like how would they configure the package and start using?

tk26
tk26 previously approved these changes Feb 2, 2022
@slashequip
Copy link
Contributor Author

@tk26 of course;

So for the most part projects have existing HTTP clients installed, so nothing more would be required - for example any project with Guzzle 7 installed would have support out of the box (so not just Laravel, Laravel was just the one I tested with).

For a project that doesn't have a client installed they can install one of the supported clients along side this package, that way they get to choose which client they'd like to use.

For example;

composer require trycourier/courier guzzlehttp/guzzle

The above would they work as expected, Guzzle being the most popular concrete HTTP client. I'll actually update the readme to reflect that better than my adapter example.

@slashequip
Copy link
Contributor Author

@tk26 update made, this is good to go now if you are happy with the above answer :)

@tk26
Copy link
Contributor

tk26 commented Feb 2, 2022

thanks, it makes ton of sense. I read a bit more about discovery and looks pretty dope. approved @slashequip - I'll merge and release the major version in a day or two :) want to push a quick change on the current lib version before doing a breaking change [give me 2 days before I reach back with the good news of shipping this to prod :D]

@slashequip
Copy link
Contributor Author

@tk26 awesome, thanks for the update. Feel free to ask any more questions in the mean time, if they come up 😄

I read a bit more about discovery and looks pretty dope

Yeah it's cool! Anything to help untangle dependency hell for consumers 😅

@tk26
Copy link
Contributor

tk26 commented Feb 2, 2022

@tk26 awesome, thanks for the update. Feel free to ask any more questions in the mean time, if they come up 😄

I read a bit more about discovery and looks pretty dope

Yeah it's cool! Anything to help untangle dependency hell for consumers 😅

💯 Very true

@tk26 tk26 merged commit d1bc529 into trycourier:main Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Potentially find a widely used alternative of Capsule
2 participants