api: throw an error if an application use a video element in multiple instance of the RxPlayer #1394
Conversation
element in multiple instance of the RxPlayer The RxPlayer was not designed to run multiple instance of RxPlayer with the same videoElement. Having multiple instance sharing the same video element would result in various issues and unexpected behavior. This commit adds a check to throw an error if a videoElement is shared between RxPlayer instances. This will provide better guidance to application developers regarding any errors in their implementation.
Florent-Bouisset
changed the title
in the demo. Commit 57fd4b9 added an error message if the video element is reused between RxPlayer instance. This was the case on the demo, and this commit ensure to dispose the previous instance before creating a new one.
|
Even if I like the improvement, this may break usage where application codes didn't destroy previous instances, though it would probably have lead to minor issues before like the one we have seen. For example, even our demo page would now be broke if we didn't update it. I'm not even sure the API documentation was clear enough about the one RxPlayer per video element rule, so we may have to make it very clear in our release note that this may break some previous usages. What do you think about this? |
I was wondering too if this should be considered as a breaking change. |
|
I liked you suggestion to only display a |
| }); | ||
|
|
||
| describe("Player instantiation", () => { | ||
| it("should throw an error if creating two players attached to the same video element", () => { |
There was a problem hiding this comment.
Wording of the test case to update.
Same for the next one
src/main_thread/api/public_api.ts
Outdated
| * @throws Error - Throws if the element is already used by another player instance. | ||
| */ | ||
| private static _priv_registerVideoElement(videoElement: HTMLMediaElement) { | ||
| const errorMessage = |
There was a problem hiding this comment.
Why declaring the errorMessage variable outside the if?
It would make more sense when reading if it comes after the related check, no?
There was a problem hiding this comment.
OK I changed it
… instance of the RxPlayer (#1394) * api: throw and error if an application use a video element in multiple instance of the RxPlayer The RxPlayer was not designed to run multiple instance of RxPlayer with the same videoElement. Having multiple instance sharing the same video element would result in various issues and unexpected behavior. This commit adds a check to throw an error if a videoElement is shared between RxPlayer instances. This will provide better guidance to application developers regarding any errors in their implementation. * demo: dispose the previous RxPlayer instance before creating a new one in the demo. Commit 57fd4b9 added an error message if the video element is reused between RxPlayer instance. This was the case on the demo, and this commit ensure to dispose the previous instance before creating a new one. * add unit test and console.warn instead of throwing to prevent breaking change in API * lint * PR feedback * format
The RxPlayer was not designed to run multiple instance of RxPlayer with the same videoElement. Having multiple instance sharing the same video element would result in various issues and unexpected behavior.
This commit adds a check to throw an error if a videoElement is shared between RxPlayer instances. This will provide better guidance to application developers regarding any errors in their implementation.
Relates to #1390