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

keep consistent video ID regardless the method executed #5411

Closed
alexrqs opened this issue Sep 5, 2018 · 5 comments · Fixed by #5415 · May be fixed by CleverTap/vue-video-player#3
Closed

keep consistent video ID regardless the method executed #5411

alexrqs opened this issue Sep 5, 2018 · 5 comments · Fixed by #5415 · May be fixed by CleverTap/vue-video-player#3

Comments

@alexrqs
Copy link
Contributor

alexrqs commented Sep 5, 2018

Description

The ID of the video instance changes after reset(), is a small change but it should be consistent.
https://jsbin.com/fisoyabuve/edit?html,console,output

Steps to reproduce

  1. Go to the js bin from the description
  2. Open the console
  3. hit play and wait for 5s

Results

Expected

if not the inial class of the video tag a consistent all lowercase id

Actual

it Capitalizes the previously lowercase tech

Additional Information

versions

videojs 7.0

browsers all

OSes all

plugins none

screen shot 2018-09-05 at 9 42 33 am
screen shot 2018-09-05 at 9 54 39 am

@welcome
Copy link

welcome bot commented Sep 5, 2018

👋 Thanks for opening your first issue here! 👋

If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.
To help make it easier for us to investigate your issue, please follow the contributing guidelines.

@alexrqs
Copy link
Contributor Author

alexrqs commented Sep 5, 2018

I narrowed down to https://github.com/videojs/video.js/blob/master/src/js/player.js#L948 and I'll be happy to create a PR I just need confirmation to: changing this line is the way to go?

@alex-barstow
Copy link
Contributor

Hm, I believe they should be the same, although @gkatsev will need to chime in on this one. If they are supposed to match, the fix will depend on whether it should be uppercase or lowercase. If it should be lowercase, the fix will be changing titleTechName to camelTechName in the line you linked above. If it should be uppercase, the fix will be updating the string here: https://github.com/videojs/video.js/blob/master/src/js/player.js#L623

@misteroneill
Copy link
Member

misteroneill commented Sep 5, 2018

Looks like line 623 is older, so it should be considered correct - so, lower-case. Really, I think line 948 should be using ${camelTechName} as @alex-barstow said and 623 should be using ${camelTechName}_api. Scratch that second suggestion, could be considered a breaking change.

Ideally, these would be unified into a single function - maybe a static method on the Tech class - so we don't encounter this sort of issue again.

@alecsgone A PR would be very welcome here! 😄

@gkatsev
Copy link
Member

gkatsev commented Sep 5, 2018

Yeah, it should be camelTechName. Technically, we already potentially broke people when we made that change to line 948. Given that this is internal, I'm ok with making that change and if it breaks anyone, we can revert and patch.
I'm not sure that a helper method would've helped here given that we have two separate usages for tech names.

gkatsev pushed a commit that referenced this issue Sep 7, 2018
Change the constant used in the techId prop to the camelCased version, this will result on consistent id when loadTech_() is executed.

Fixes #5411
jankrueger added a commit to jankrueger/video.js that referenced this issue Sep 27, 2018
Prevent video.js from throwing errors when reloading the tech a after using reset().
This was already fixed for version 7 here: videojs#5415

Fixes videojs#5411
gkatsev pushed a commit that referenced this issue Sep 28, 2018
Prevent video.js from throwing errors when reloading the tech a after using reset().
This was already fixed for version 7 here: #5415

Fixes #5411 for 6.x
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants