fix: better evented validation and error messages #6982
Conversation
| @@ -61,33 +62,76 @@ QUnit.test('evented() with custom element', function(assert) { | |||
| ); | |||
| }); | |||
|
|
|||
| QUnit.test('trigger() errors', function(assert) { | |||
There was a problem hiding this comment.
We never had a test for this before (since we didn't have an errors). I added it.
brandonocasey
force-pushed
the
fix/better-evented-validation
branch
from
b3bba8a to
52e12b8
Compare
Follow up from #6982. We previously threw an error but we've seen it happen unexpectedly. Instead, we should log an error. Here, if we have a `log` object on the current object, we should use it, otherwise, we use a default `log` object.
Follow up from #6982. We previously threw an error, but we've seen it happen unexpectedly. Instead, we should log an error. We will still throw an error if the event is undefined or null. Here, if we have a `log` object on the current object, we should use it, otherwise, we use a default `log` object.
|
This broke something in my plugin (using video.js 7.11.4). Investigating.. |
|
In 7.11.3, we had it throw in all error cases, in 7.11.4 we only throw in a few cases #7067. |
|
@gkatsev it actually uncovered an issue in my tests: there was no player passed into constructor of custom component. So this is a good change for video.js so far. I fixed it here: collab-project/videojs-record@493440b#diff-fd3e579b7fcc86745372fdfc089275daa8bc05c106d2ae363004114511d04481 |
|
@thijstriemstra ah, cool! So, no further action needed on our end right now? |
|
nope! |
Follow up from videojs#6982. We previously threw an error, but we've seen it happen unexpectedly. Instead, we should log an error. We will still throw an error if the event is undefined or null. Here, if we have a `log` object on the current object, we should use it, otherwise, we use a default `log` object.
Our current error messages and validation are not enough as some errors can still seek through. I improved our error handling and reporting in many places and will make code comments to show why a change was made below. Originally this was a part of #6967