Skip to content

Clean up event loop from client #58

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

Merged

Conversation

WyriHaximus
Copy link
Collaborator

Q A
Bug fix? no
New feature? no
BC breaks? yes
Deprecations? no
License MIT

@dbu
Copy link
Contributor

dbu commented Nov 14, 2022

thanks. i guess we don't care about the roave BC break complaint - the 4.x branch is allowed to have BC breaks. but can you explicitly mention this in the changelog?

there seems to be a place where loop is still referenced in the Client.php, according to phpstan. can you check that please?

and can you please again do the cs fix?

i merged #56, so this can be rebased on 4.x to have the last state.

@WyriHaximus
Copy link
Collaborator Author

Clean all failing checks up before opening it for review

@WyriHaximus WyriHaximus force-pushed the 4.x-clean-up-event-loop-from-client branch 2 times, most recently from f88ec78 to 7d03940 Compare November 14, 2022 10:34
@WyriHaximus WyriHaximus marked this pull request as ready for review November 14, 2022 10:35
@WyriHaximus
Copy link
Collaborator Author

thanks. i guess we don't care about the roave BC break complaint - the 4.x branch is allowed to have BC breaks. but can you explicitly mention this in the changelog?

Yes let me update this PR with an explanation in the changelog before you merged it

there seems to be a place where loop is still referenced in the Client.php, according to phpstan. can you check that please?

and can you please again do the cs fix?

i merged #56, so this can be rebased on 4.x to have the last state.

Done, done, and done.

@WyriHaximus
Copy link
Collaborator Author

WyriHaximus commented Nov 14, 2022

Also, where is the documentation for this package located? Will also update those while I'm at it :).

Nvm it's at the top of the page at https://docs.php-http.org/en/latest/clients/react-adapter.html

@WyriHaximus WyriHaximus force-pushed the 4.x-clean-up-event-loop-from-client branch from 7d03940 to 454f4d0 Compare November 14, 2022 10:59
CHANGELOG.md Outdated
@@ -25,8 +25,19 @@ async(static function () {
})();
```

Another major change in this release is that injecting the event loop has been removed, and now fully uses
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Another major change in this release is that injecting the event loop has been removed, and now fully uses
Another major change in this release is that you no longer inject the event loop into the client. It now only uses

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated 👍

Fully relying on Loop::get() from here on, as such bumped the react/http and react/event-loop to compatible versions.
@WyriHaximus WyriHaximus force-pushed the 4.x-clean-up-event-loop-from-client branch from 454f4d0 to cc97aab Compare November 14, 2022 11:09
@dbu dbu merged commit 4497786 into php-http:4.x Nov 14, 2022
@dbu
Copy link
Contributor

dbu commented Nov 14, 2022

shall i tag now, or do you plan other changes?

@WyriHaximus WyriHaximus deleted the 4.x-clean-up-event-loop-from-client branch November 14, 2022 11:29
@WyriHaximus
Copy link
Collaborator Author

Go for it, I'm currently updating the documentation, which seems to be "somewhat" out of date 😅

@dbu
Copy link
Contributor

dbu commented Nov 14, 2022

here we go: https://github.com/php-http/react-adapter/releases/tag/4.0.0

do you want commit rights on the react-adapter so you can create releases and merge changes? i am happy to keep doing reviews for general sanity and understandability, but have very little experience with reactphp and event loops.

@WyriHaximus
Copy link
Collaborator Author

here we go: https://github.com/php-http/react-adapter/releases/tag/4.0.0

🎉

do you want commit rights on the react-adapter so you can create releases and merge changes? i am happy to keep doing reviews for general sanity and understandability, but have very little experience with reactphp and event loops.

Sure 👍

@dbu
Copy link
Contributor

dbu commented Nov 15, 2022

great, thanks for this. i sent you the invite.

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.

2 participants