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

Promise v3 #1157

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Promise v3 #1157

wants to merge 10 commits into from

Conversation

key2peace
Copy link
Collaborator

No description provided.

- changed ExtendedPromiseInterface to PromiseInterface
- replaced Discord\Helpers\Deferred with React\Promise\Deferred
- changed ->done(true function, false function) with ->then(true function)->catch(false function)
- changed ->always() to ->finally()
- added use React\Promise\Deferred where I think it was missing
- updated $deferred->resolve() to $deferred->resolve(true)
- updated $deferred->reject() to $deferred->reject(throwable)
@key2peace key2peace marked this pull request as draft July 17, 2023 14:45
@SQKo SQKo added the dependencies Pull requests that update a dependency file label Jul 22, 2023
@2colours
Copy link

@key2peace what is the state of this PR? Being stuck with v2 Promises (thanks, Composer, for not handling multiple versions of one package btw) means even something as banal as set_rejection_handler cannot be used.

@SQKo
Copy link
Member

SQKo commented Mar 16, 2024

@key2peace what is the state of this PR? Being stuck with v2 Promises (thanks, Composer, for not handling multiple versions of one package btw) means even something as banal as set_rejection_handler cannot be used.

It wont be in v10, many dependencies still stuck with v2 promises
As well compatibility issue with future php 8.4 version, which they'll promise to support by reactphp v3

about set_rejection_handle, Just be careful with your promise codes for now
always use then() followed by argumentless and empty done() so you can refactor your code easier in future

@2colours
Copy link

2colours commented Mar 16, 2024

@SQKo DiscordPHP should at the very least handle the rejection path with a logger, with a log level >= warning. There is no reason to not do that since afterwards nobody could even catch the rejection currently. I think that's way better than leaving it up to the consumers to develop some quasi framework that minimizes the boilerplate for handling this for each and every registered command.

I'm already looking into the code; it's just not easy to navigate around with something you have only been actively using for like 2 months...

@2colours
Copy link

see #1208

This is anything but exhaustive, it just covers my most obvious use case.

@SQKo
Copy link
Member

SQKo commented Mar 16, 2024

@SQKo DiscordPHP should at the very least handle the rejection path with a logger, with a log level >= warning. There is no reason to not do that since afterwards nobody could even catch the rejection currently. I think that's way better than leaving it up to the consumers to develop some quasi framework that minimizes the boilerplate for handling this for each and every registered command.

I'm already looking into the code; it's just not easy to navigate around with something you have only been actively using for like 2 months...

We know about it already, unfortunately the issue we are facing might be also caused by dependencies, our code is fine when web socket is not connected yet..

Regarding to your suggestion, we already considered about it, but it is wrong to log everything even those promise rejections already handled/caught by users and not respecting users configured Logger.
So we went to a better idea, It is somewhat already implemented but if i recall, it requires your code to be wrapped inside a coroutine yields so your own code throws will be also forwarded:

if ($e instanceof \Error) {
throw $e;
} elseif ($e instanceof \Exception) {
$this->logger->error('exception while trying to handle dispatch packet', ['packet' => $data->t, 'exception' => $e]);
} else {
$this->logger->warning('rejection while trying to handle dispatch packet', ['packet' => $data->t, 'rejection' => $e]);
}

notice why it is logged only in websocket events because it only gets silence after websocket connected...

Due to the nature of promise chain, where this library being in middle position, the ideal solution has to be implemented on user's end.
It's not true that nobody can even catch the rejection, thats why I previously remind the proper use of then() and done(). Same for this library perspective, if the dependency does not handle promise rejection properly, it can never be caught by us.

@discord-php discord-php locked as off-topic and limited conversation to collaborators Mar 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants