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

Fixes instanceof JitsiTrackError #309

Merged
merged 1 commit into from
Nov 14, 2016
Merged

Conversation

hristoterezov
Copy link
Member

@hristoterezov hristoterezov commented Nov 9, 2016

JitsiTrackError extends Error. Due to an issue with babel, extending built-in classes seems not very good idea(babel/babel#3083).

This PR fixes instanceof JitsiTrackError calls.

@paweldomas paweldomas self-assigned this Nov 14, 2016
@@ -29,109 +29,111 @@ TRACK_ERROR_TO_MESSAGE_MAP[JitsiTrackErrors.TRACK_MUTE_UNMUTE_IN_PROGRESS]
TRACK_ERROR_TO_MESSAGE_MAP[JitsiTrackErrors.NO_DATA_FROM_SOURCE]
= "The track has stopped receiving data from it's source";


// FIXME: Using prototype inheritance because otherwise instanceof is not
Copy link
Member

Choose a reason for hiding this comment

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

The comment is a bit misleading about the bug. From the issues description one can figure out that it's not recommended to extend a built-in type (and so does the PR description), but the comment looks like we're waiting for the bug to be fixed.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the bug got fixed I think it's gonna be better to return back to the ES6 syntax. Do you want me to include this information in the comment?

Copy link
Member

Choose a reason for hiding this comment

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

No, I got the code wrong and thought that we will not longer extend Error - it's ok as it is.

/**
*
* Represents an error that occurred to a JitsiTrack. Can represent various
* types of errors. For error descriptions (@see JitsiTrackErrors).
*
* @extends Error
Copy link
Member

@paweldomas paweldomas Nov 14, 2016

Choose a reason for hiding this comment

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

It does not extend the Error anymore, does it ?

Do we have any usages of JitsiTrackError that rely on that fact ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It still extends Error. See the lines after the constructor:

JitsiTrackError.prototype = Object.create(Error.prototype);
JitsiTrackError.prototype.constructor = JitsiTrackError;

I'm not sure where and why we are using this but even if we are not using it I think it's good to extend Error since we are reporting error.

Copy link
Member

Choose a reason for hiding this comment

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

ok, makes sense, thanks for the explanation !

@paweldomas paweldomas merged commit 4b4e226 into master Nov 14, 2016
@hristoterezov hristoterezov deleted the fix_jitsi_track_error branch November 14, 2016 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants