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

Updates the default version of dash.js, hls.js #1056

Merged
merged 1 commit into from
Nov 14, 2020

Conversation

jrstinson
Copy link
Contributor

The default versions of hls.js and dash.js used are several releases out of date, and so by default are missing several good features. While it is possible to specify different versions for these in the configuration, I think it is valuable to keep the default versions being used up to date.

The default versions of hls.js and dash.js used are several releases out of date, and so by default are missing several good features. While it is possible to specify different versions for these in the configuration, I think it is valuable to keep the default versions being used up to date.
hlsVersion: '0.13.1',
dashVersion: '2.9.2',
hlsVersion: '0.14.16',
dashVersion: '3.1.3',
Copy link
Owner

Choose a reason for hiding this comment

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

My concern here is if people are using ReactPlayer with some specific dash functionality that breaks in 3.1.3 because of the major bump. Are we ok with potentially breaking some implementations in a non-major bump of react-player?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a fair concern. Perhaps I can do some research into whether there are any known breaking changes in these releases (which are latest), and if so whether or not there is a more recent non-breaking release. In my opinion it would be better to default to the most recent stable version and be able to configure your way to a previous version instead of the other way around, but I understand the concern with breaking current implementations. My fix here which just changes the default version to a newer one also isn't the only option- it is also possible to default to using the latest version of both by using http://cdn.dashjs.org/latest/dash.all.min.js and https://cdn.jsdelivr.net/npm/hls.js@latest. This still runs into your concern about breaking existing implementations, but would also prevent anyone from having to update the specific version in this repo anymore.

Copy link
Owner

Choose a reason for hiding this comment

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

it is also possible to default to using the latest version of both by using http://cdn.dashjs.org/latest/dash.all.min.js and https://cdn.jsdelivr.net/npm/hls.js@latest. This still runs into your concern about breaking existing implementations, but would also prevent anyone from having to update the specific version in this repo anymore.

Yeah, this would just expose ReactPlayer to breaking changes for existing implementations, which wouldn't be too great.

I've had a quick look at https://github.com/Dash-Industry-Forum/dash.js/releases/tag/v3.0.0 and there's stuff like Dash-Industry-Forum/dash.js#2954 that removes deprecated methods. Basically any ReactPlayer implementation that uses anything deprecated would suddenly break. Maybe it's worth just taking the hit? I'm not really sure what's best.

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 a pickle, for sure. It seems worth it to me to at least upgrade to Dash v3, as there were some pretty major changes there. The main reason I bring this up is just because at this point, there have been enough changes to dash.js and hls.js since 2.9.2 and 0.13.1 that anyone now implementing react-player would have to hard code in a more recent version to get what I would say is the "expected" functionality. I myself am here for that very reason- I was having weird issues running a dash stream in someone else's implementation of react-player, which turned out to be using the older versions. I don't know for sure whether it's the right call or not, but it does seem like something that should be considered at some point so that people implementing react-player can get expected functionality/features without having to specify a newer version themselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cookpete In my opinion, whenever users who are using the ReactPlayer are going to update the version of ReactPlayer, it is their responsibility to make sure that whether new version is gonna work as expected with their implementation. Infact, this is a general rule of updating any dependency in a developed project. So, I think, we can update default versions to the latest.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah I think you're right. I'm happy to bump these in a minor version bump and just hope it doesn't affect too many people negatively.

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

3 participants