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

fix(FEC-11035): playback error dispatch from player instead of from engine #545

Merged
merged 7 commits into from
Mar 11, 2021

Conversation

Yuvalke
Copy link
Contributor

@Yuvalke Yuvalke commented Feb 24, 2021

Description of the Changes

Issue: engine doesn't expose an error for playback error, it dispatched from the player instead - the engine decorator can't ignore the playback error.
Solution: change the dispatch from the player to the engine to make sure the playback error could reject on the engine decorator.
tests failed for scenario _nativeTextTracksMap has index 0 and 2(metadata between) so for raise an error when it gets to i = 1 that doesn't exist.

CheckLists

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • test are passing in local environment
  • Travis tests are passing (or test results are not worse than on master branch :))
  • Docs have been updated

@Yuvalke Yuvalke changed the title fix error handling fix: playback error doesn't dispatch from engine in some case Feb 24, 2021
@Yuvalke Yuvalke changed the title fix: playback error doesn't dispatch from engine in some case fix(FEC-11035): playback error dispatch from player instead of from engine Feb 24, 2021
@Yuvalke Yuvalke marked this pull request as ready for review February 24, 2021 16:48
@Yuvalke Yuvalke requested a review from a team February 24, 2021 17:07
@Yuvalke Yuvalke self-assigned this Feb 24, 2021
})
.catch(error => {
this.dispatchEvent(new FakeEvent(Html5EventType.ERROR, error));
.finally(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

not supported in IE11, please verify we transpile this.
https://caniuse.com/promise-finally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's OK, we're using it with post-roll on ads controller as well, tested it as well on IE11

@Yuvalke Yuvalke merged commit 848bf1b into master Mar 11, 2021
@Yuvalke Yuvalke deleted the error-handling branch March 11, 2021 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants