search

discord-php / DiscordPHP

An API to interact with the popular messaging app Discord
MIT License
977 stars 236 forks source link

Promise v3 #1157

Open key2peace opened 1 year ago

2colours commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago

see https://github.com/discord-php/DiscordPHP/pull/1208

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

SQKo commented 5 months ago

@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:

https://github.com/discord-php/DiscordPHP/blob/ac16fdf6e9a9d20003edbca734c386ddcf616eda/src/Discord/Discord.php#L793-L799 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.