Skip to content

AbortHandler's timeout should always be cleared#474

Merged
Rapha0511 merged 1 commit intodailymotion:masterfrom
olafbuitelaar:patch-1
Sep 5, 2024
Merged

AbortHandler's timeout should always be cleared#474
Rapha0511 merged 1 commit intodailymotion:masterfrom
olafbuitelaar:patch-1

Conversation

@olafbuitelaar
Copy link
Copy Markdown
Contributor

the timeout should always be cleared even if an error occurred within the promise

Description

When you provide a custom urlHandler and an error occurs here-in it could be possible the abortHandlers timeout isn't cleared, causing the abortHandler to trigger even if the request is done.

Type

  • [ x] Fix

the timeout should always be cleared even if an error occurred within the promise
@olafbuitelaar olafbuitelaar changed the title Update url_handler.js AbortHandler's timeout should always be cleared Jul 18, 2024
Copy link
Copy Markdown
Contributor

@Rapha0511 Rapha0511 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello!
Thank you for reporting this issue!
I left some comment.

Comment on lines +70 to +71
.finally(()=>{
clearTimeout(timer);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be done in a finally block after the catch ? since .finally() is usually used in a Promise chain

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the await isn't that different that a promise chain, only different syntax. if you would want to move it to the try..catch..finally. the const timer needs to be moved outside the try block since it's block scoped. probably all together less neat.

@olafbuitelaar
Copy link
Copy Markdown
Contributor Author

any update on this? i hoped it would have been included in 6.0.1

@Rapha0511
Copy link
Copy Markdown
Contributor

Hello @olafbuitelaar
I Accept this solution, and it will be included in the next release :)

@Rapha0511 Rapha0511 merged commit 5d148f7 into dailymotion:master Sep 5, 2024
@olafbuitelaar
Copy link
Copy Markdown
Contributor Author

when is it planned to publish this?

@ZacharieTFR
Copy link
Copy Markdown
Contributor

when is it planned to publish this?

It's ready to use! 🎉 Available in 6.0.2 https://github.com/dailymotion/vast-client-js/releases/tag/6.0.2

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants