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

Avoid finally in promise chain. Log error if error happen in _l_lastVideoSenderUpdatePromise. #2465

Closed
wants to merge 2 commits into from

Conversation

Romick2005
Copy link
Contributor

Fixes #2464 _lastVideoSenderUpdatePromise.finally is not a function issue. Add more information about occurred errors.

@jitsi-jenkins
Copy link

Hi, thanks for your contribution!
If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

@saghul
Copy link
Member

saghul commented Feb 23, 2024

What environment are you running in? Is Promise.prototype.finally not defined there?

@Romick2005
Copy link
Contributor Author

What environment are you running in? Is Promise.prototype.finally not defined there?

react-native

@saghul
Copy link
Member

saghul commented Feb 23, 2024

Then you need to apply the promise finally polyfill.

@saghul saghul closed this Feb 23, 2024
@Romick2005
Copy link
Contributor Author

Romick2005 commented Feb 23, 2024

Then you need to apply the promise finally polyfill.

But this pr allow us to see errors also. And this is the only place where we can have more environments covered. Wouldn't it be better? Also this will cover some case with UnhandledPromiseRejection. So for example:

const p = Promise.reject(new Error('BOOM'));
p.finally(() => {});

will throw UnhandledPromiseRejectionWarning
(node:22503) UnhandledPromiseRejectionWarning: Error: BOOM at Object.<anonymous>

But this:

const p = Promise.reject(new Error('BOOM'));
p.catch(() => {}).finally(() => {});

image

So it is not also about polyfills, but for general code reliability.
nodejs/node#28636
@saghul What do you think?

@saghul
Copy link
Member

saghul commented Feb 24, 2024

Error in that function can happen and are ok to ignore, hence the finally.

While what you say is generally good advice, it doesn't need to be followed in every case.

@Romick2005
Copy link
Contributor Author

Is this a good case to kill two rabbits with one shot? Allow easy react-native integration and have proper error handling and logs?

I do not agree with you. So I will patch it, sorry.

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.

_lastVideoSenderUpdatePromise.finally is not a function
3 participants