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

Should fix the UnhandledPromiseRejectionWarning #201 #225 #227 #229 #230

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Schmoopiie
Copy link
Member

@Schmoopiie Schmoopiie commented May 24, 2017

I need some people to test this change and if everything goes well, I will land it on v1.2.2. I know that we will need polyfills for browser support and for backward compatibility for Node < 6. The main goal is to fix the deprecation warning when using Node 6, 7 and 8 (which is scheduled to be released on May 30, 2017).

@ThatLurker
Copy link

Any update on this?

@Yerrak
Copy link

Yerrak commented Jan 24, 2019

Ran into a bunch of unhandled promise rejections today and it kind of pollutes my logs...
So, here is my attempt at getting this PR back on track.

Testing this PR in my simple use case of multiple times calling client.join() confirmed that it would work, assuming the caller consumes both, resolve and reject, like so:

client.join(channel).then((value) => {
    // [channelName]
}, (reason) => {
    // No response from Twitch.
});

What kind of confused me was that I was able to observe that the action (joining the channel in this case) was able to complete even though the promise was rejected due to losing the race against the delay.
The _promiseJoin was emitted shortly after the delay passed and processed nonetheless. This makes handling the rejection No response from Twitch kind of useless as it is not definitive.
Anyone has any insight into this behaviour (and possibly an already existing solution I missed)?

Because of the above, the approach of just catching the rejection as proposed here #201 also appeals to me - but as discussed over there, we would not be able to handle the promise result in the actual application that uses tmi.js.

I'm not quite familiar with compatibility considerations, but Promise.race() should work everywhere (except for IE of course).

(Reducing the promise delay to < 200 seems to be a nice and easy way to test this behaviour.)

@liquidvisual
Copy link

What's happening with this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants