Use react/async for promise wait #50

WyriHaximus · 2022-04-24T23:04:41Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Related tickets	fixes #49
Documentation	if this is a new feature, link to pull request in https://github.com/php-http/documentation that adds relevant documentation
License	MIT

What's in this PR?

Change promise implementation to not block

Why?

Avoid blocking, see #49

Checklist

Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
Documentation pull request created (if not simply a bugfix)

dbu · 2022-05-06T15:05:36Z

please let me know if you need any input (i have no experience with react though)

WyriHaximus · 2022-05-06T16:17:44Z

@dbu Will do! I think the only thing might be the @dev targeting in composer.json, but lets get the build green first. Will also update the workflow to run 8.1 and drop the older version. Will ping you when I need something from your end.

WyriHaximus · 2022-05-09T15:11:09Z

@dbu Filed #51 and #52 to run tests against PHP 8.1, one is a simple addition of 8.1, while the other finds the supported PHP versions in range for the project. Take your pick :D

dbu · 2022-06-29T05:42:02Z

src/Promise.php

-            $loop->run();
+        try {
+            await($this->promise);
+        } catch (\Throwable) {


is it correct to eat away the exception and not do anything with it? seems dangerous to me.

Check lines 82 to 96, and 123 to 129. In theory that will cover the swallowing here. This is something I'm considering changing but not sure what the best approach is yet

ah, i see yep. thanks for the explanation.

what happens with line 128 if there was an exception and we chose not to unwrap? the response will be null, right? but that will have been the same before if i understand correctly.

Yes, I kept the behavior the same as before. Not unwrapping is a very conscious call to swallow any error that might come up.

dbu · 2022-07-13T06:22:12Z

i fixed the static analysis in the master branch. can you please rebase?

what is needed to wrap this MR up?

WyriHaximus · 2022-07-13T13:30:56Z

i fixed the static analysis in the master branch. can you please rebase?

Will do so tonight 👍

what is needed to wrap this MR up?

A stable release of react/async which is the case since Monday

dbu · 2022-07-13T21:03:09Z

what is needed to wrap this MR up?

A stable release of react/async which is the case since Monday

ah nice! i will now be away until next week, will check back then and tag a release of this adapter if its ready.

WyriHaximus · 2022-07-14T16:37:12Z

i fixed the static analysis in the master branch. can you please rebase?

@dbu Ok so funny thing, it seems that the static analysis runs on PHP 8.0, and this PR raises the minimum PHP version to 8.1 😂 . Code coverage has a similar issue, but that is running on PHP 7.4.

dbu · 2022-07-28T06:02:14Z

@dbu Ok so funny thing, it seems that the static analysis runs on PHP 8.0, and this PR raises the minimum PHP version to 8.1 joy . Code coverage has a similar issue, but that is running on PHP 7.4.

can you please adjust the php version for code coverage?

with static analysis its a bit more tricky, as we use the oskarstark phpstan image. i created OskarStark/phpstan-ga#53 to try to upgrade to php 8.1

WyriHaximus · 2022-07-28T18:50:56Z

@dbu Ok so funny thing, it seems that the static analysis runs on PHP 8.0, and this PR raises the minimum PHP version to 8.1 joy . Code coverage has a similar issue, but that is running on PHP 7.4.

can you please adjust the php version for code coverage?

Oops I missed that was hardcoded. Just updated that.

with static analysis its a bit more tricky, as we use the oskarstark phpstan image. i created OskarStark/phpstan-ga#53 to try to upgrade to php 8.1

How open are you to alternatives? Could look into using a github action for it.

dbu · 2022-07-29T13:16:49Z

How open are you to alternatives? Could look into using a github action for it.

the thing we use is a github action ;-)

oskar already merged the upgrade and tagged it. i now restarted the build and its green \o/

the codestyle error is because it wants the use function to have a newline before and after - maybe put the use function after the list of classes we use?

Move and also bump phpunit/phpunit to require dev as it was wrongfully place in require in php-http#52.

WyriHaximus · 2022-07-30T15:48:27Z

How open are you to alternatives? Could look into using a github action for it.

the thing we use is a github action ;-)

Oh hah it looked like it was just a docker container.

oskar already merged the upgrade and tagged it. i now restarted the build and its green \o/

Great!

the codestyle error is because it wants the use function to have a newline before and after - maybe put the use function after the list of classes we use?

Tried that, but it wants the empty lines before and after it in the current position

dbu · 2022-07-31T09:11:58Z

sorry about the use function thing. not sure why they do it like that, its a bit silly. but i rather have it look a bit silly than open the can of worms of discussing about codestyle 😄

thanks a lot for the contribution, i will merge now. should this be a version 3 or does the change merit a major version bump?

and should i already tag a release, or are you doing more testing?

WyriHaximus · 2022-08-01T16:29:15Z

sorry about the use function thing. not sure why they do it like that, its a bit silly. but i rather have it look a bit silly than open the can of worms of discussing about codestyle smile

No worries, I respect and follow the CS rules of the project without complaining, how silly they might look to me 😃 .

thanks a lot for the contribution, i will merge now. should this be a version 3 or does the change merit a major version bump?

Would go for a major version bump IMHO because the mechanic of how you wait changes and now requires fibers.

and should i already tag a release, or are you doing more testing?

Give me a week to test it in a project. Need to set up Sentry for it, should be fun :D

dbu · 2022-08-02T06:16:41Z

great! i merged and set up the 4.x branch for it (getting rid of confusing & politically incorrect master branch while i am at it)

can you please update the changelog with more details and maybe a short instruction how to adapt usage?

and let me know when to tag a release ;-)

WyriHaximus · 2022-11-13T14:11:43Z

and should i already tag a release, or are you doing more testing?

Give me a week to test it in a project. Need to set up Sentry for it, should be fun :D

That took a bit longer than intended sorry. But I come bearing the goodest of news

great! i merged and set up the 4.x branch for it (getting rid of confusing & politically incorrect master branch while i am at it)

👍

can you please update the changelog with more details and maybe a short instruction how to adapt usage?

Will do 👍.

and let me know when to tag a release ;-)

Let me file a changelog update and we're good to go. I've tested the 4.x branch in a project for Sentries HTTP transport. Running without any issues what so ever <3.

I somehow missed those while working on php-http#50

WyriHaximus · 2022-11-13T22:09:22Z

Ok PR for fibers explanation in the changelog is up at #57 and also filed #56 with some clean up

I somehow missed those while working on php-http#50

WyriHaximus force-pushed the use-react-async-for-promise-wait branch from db364c9 to e2198b6 Compare April 25, 2022 17:08

WyriHaximus force-pushed the use-react-async-for-promise-wait branch from e2198b6 to 2fff833 Compare May 2, 2022 21:43

This was referenced May 9, 2022

Test against PHP 8.1 #51

Closed

Detect supported PHP versions in range during CI instead of hardcoding them #52

Merged

WyriHaximus force-pushed the use-react-async-for-promise-wait branch 2 times, most recently from efe7b8c to a4574ed Compare June 28, 2022 21:35

dbu reviewed Jun 29, 2022

View reviewed changes

WyriHaximus force-pushed the use-react-async-for-promise-wait branch from a4574ed to e4f58f3 Compare July 11, 2022 19:13

WyriHaximus force-pushed the use-react-async-for-promise-wait branch from e4f58f3 to bbd20b0 Compare July 14, 2022 14:35

WyriHaximus changed the title ~~[WIP] Use react/async for promise wait~~ Use react/async for promise wait Jul 14, 2022

WyriHaximus marked this pull request as ready for review July 14, 2022 17:34

dbu mentioned this pull request Jul 28, 2022

update to php 8.1 OskarStark/phpstan-ga#53

Merged

WyriHaximus force-pushed the use-react-async-for-promise-wait branch from bbd20b0 to d4dab3b Compare July 28, 2022 18:49

WyriHaximus force-pushed the use-react-async-for-promise-wait branch from d4dab3b to 7e14dfd Compare July 30, 2022 13:48

Use react/async for promise wait

2dcaf60

Move and also bump phpunit/phpunit to require dev as it was wrongfully place in require in php-http#52.

WyriHaximus force-pushed the use-react-async-for-promise-wait branch from 7e14dfd to 2dcaf60 Compare July 30, 2022 13:59

dbu merged commit f6681f2 into php-http:master Aug 2, 2022

WyriHaximus deleted the use-react-async-for-promise-wait branch November 13, 2022 13:59

WyriHaximus added a commit to WyriHaximus-labs/react-adapter that referenced this pull request Nov 13, 2022

Clean up event loop left overs from promise

4773b70

I somehow missed those while working on php-http#50

This was referenced Nov 13, 2022

Clean up event loop left overs from promise #56

Merged

Explain fibers in changelog #57

Merged

WyriHaximus added a commit to WyriHaximus-labs/react-adapter that referenced this pull request Nov 14, 2022

Clean up event loop left overs from promise

421bb9b

I somehow missed those while working on php-http#50

WyriHaximus added a commit to WyriHaximus-labs/react-adapter that referenced this pull request Nov 14, 2022

Clean up event loop left overs from promise

931ef9a

I somehow missed those while working on php-http#50

WyriHaximus mentioned this pull request Nov 14, 2022

Update react adapter to v4 php-http/documentation#299

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use react/async for promise wait #50

Use react/async for promise wait #50

WyriHaximus commented Apr 24, 2022 •

edited by dbu

dbu commented May 6, 2022

WyriHaximus commented May 6, 2022

WyriHaximus commented May 9, 2022

dbu Jun 29, 2022

WyriHaximus Jun 29, 2022

dbu Jul 13, 2022

WyriHaximus Jul 13, 2022

dbu commented Jul 13, 2022

WyriHaximus commented Jul 13, 2022

dbu commented Jul 13, 2022

WyriHaximus commented Jul 14, 2022 •

edited

dbu commented Jul 28, 2022

WyriHaximus commented Jul 28, 2022

dbu commented Jul 29, 2022

WyriHaximus commented Jul 30, 2022

dbu commented Jul 31, 2022

WyriHaximus commented Aug 1, 2022

dbu commented Aug 2, 2022

WyriHaximus commented Nov 13, 2022

WyriHaximus commented Nov 13, 2022

Use react/async for promise wait #50

Use react/async for promise wait #50

Conversation

WyriHaximus commented Apr 24, 2022 • edited by dbu

What's in this PR?

Why?

Checklist

dbu commented May 6, 2022

WyriHaximus commented May 6, 2022

WyriHaximus commented May 9, 2022

dbu Jun 29, 2022

Choose a reason for hiding this comment

WyriHaximus Jun 29, 2022

Choose a reason for hiding this comment

dbu Jul 13, 2022

Choose a reason for hiding this comment

WyriHaximus Jul 13, 2022

Choose a reason for hiding this comment

dbu commented Jul 13, 2022

WyriHaximus commented Jul 13, 2022

dbu commented Jul 13, 2022

WyriHaximus commented Jul 14, 2022 • edited

dbu commented Jul 28, 2022

WyriHaximus commented Jul 28, 2022

dbu commented Jul 29, 2022

WyriHaximus commented Jul 30, 2022

dbu commented Jul 31, 2022

WyriHaximus commented Aug 1, 2022

dbu commented Aug 2, 2022

WyriHaximus commented Nov 13, 2022

WyriHaximus commented Nov 13, 2022

WyriHaximus commented Apr 24, 2022 •

edited by dbu

WyriHaximus commented Jul 14, 2022 •

edited