fix(evented): log an error on invalid type #7067
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
gkatsev
commented
Jan 26, 2021
src/js/mixins/evented.js
Outdated
| @@ -444,7 +445,7 @@ const EventedMixin = { | |||
| const type = event && typeof event !== 'string' ? event.type : event; | |||
|
|
|||
| if (!isValidEventType(type)) { | |||
| throw new Error(`Invalid event type for ${objName(this)}#trigger; ` + | |||
| (this.log || log).error(`Invalid event type for ${objName(this)}#trigger; ` + | |||
There was a problem hiding this comment.
This allows us to use the player/plugin/etc log if it exists, for example:
> player.controlBar.trigger({})
< VIDEOJS: ERROR: Invalid event type for ControlBar#trigger; must be a non-empty string or object with a type key that has a non-empty value.
> player.trigger({})
< VIDEOJS: vjs_video_3: ERROR: Invalid event type for null#trigger; must be a non-empty string or object with a type key that has a non-empty value.
brandonocasey
approved these changes
Jan 26, 2021
| 'must be a non-empty string or object with a type key that has a non-empty value.'; | ||
|
|
||
| if (event) { | ||
| (this.log || log).error(error); |
There was a problem hiding this comment.
should we return no matter what if we don't get a valid event type?
There was a problem hiding this comment.
I think to reduce potential breakage, we want to try and continue through to the next part here and not return early.
Though, if they got here there's probably something wrong, but it could still be working in some way, for better and worse.
24 tasks
alex-barstow
added a commit
to alex-barstow/video.js
that referenced
this pull request
Apr 8, 2022
edirub
pushed a commit
to edirub/video.js
that referenced
this pull request
Jun 8, 2023
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.
edirub
pushed a commit
to edirub/video.js
that referenced
this pull request
Jun 8, 2023
…pes (videojs#7719) BREAKING CHANGE: Instead of logging an error message, invalid events will now trigger an `Error` which will terminate the call stack.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
logobject on the current object, we should use it,otherwise, we use a default
logobject.