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

feature request: expose version from player.version() #8538

Closed
Svarozic opened this issue Jan 2, 2024 · 20 comments
Closed

feature request: expose version from player.version() #8538

Svarozic opened this issue Jan 2, 2024 · 20 comments

Comments

@Svarozic
Copy link
Contributor

Svarozic commented Jan 2, 2024

I am using videojs via NPM https://videojs.com/getting-started/#install-via-npm and I cant find a way, how to access videojs.VERSION that would be normally available, if I would use videojs via CDN.

Is it possible to get version of video JS from Player instance https://docs.videojs.com/player ?

Steps to reproduce:

Copy link

welcome bot commented Jan 2, 2024

👋 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.

@amtins
Copy link
Contributor

amtins commented Jan 2, 2024

@Svarozic

Is it possible to get version of video JS from Player instance https://docs.videojs.com/player ?

No, because videojs.VERSION is the version of the library, whereas the player is a component. However, it is possible to expose the version through the player instance. Here are 3 possibilities

When instantiating the player

const player = videojs('my-player', {
vjsVersion : videojs.VERSION
});

// Access the value
player.options().vjsVersion;

Modifying the Player component prototype

videojs.getComponent('Player').prototype.VJS_VERSION = videojs.VERSION;
const player = videojs('my-player');

// Access the value
player.VJS_VERSION;

Modifying the Component component prototype

videojs.getComponent('Component').prototype.VJS_VERSION = videojs.VERSION;
const player = videojs('my-player');

// Access the value
player.VJS_VERSION;
player.controlBar.VJS_VERSION;
// ... or any other component

You can rename VJS_VERSION to VERSION if you prefer. Hope this helps.

@gkatsev
Copy link
Member

gkatsev commented Jan 2, 2024

Also, are you saying that videojs.VERSION isn't working for you? Because it should be working as far as I can tell. Can you provide a specific test case that shows it not working?

@Svarozic
Copy link
Contributor Author

Svarozic commented Jan 2, 2024

Thank you @amtins for nice workaround and 3 showcases.

@gkatsev no, because window.videojs.VERSION is not initialised and/or VERSION property does not exists on function videojs that I import from npm package (I am not using script with CDN link)

In my case, I am developing on analytics library that works with VideoJS player. I am receiving already created instance of Player, I am listening to player events and report them. I also need to get a version of VideoJS library that was used to create that VideoJS player and send it as part of that report, and that is a reason, why I have created this issue.

Until now, consumers of my library have been using CDN VideoJS integration, where I could relay on window.videojs.VERSION, but once they started to use VideoJS via npm, this version started to be reported as undefined (which makes sense). I have tried to inspect Player instance, to see, if VideoJS library version is present there, but I have not found anything, so I have decided to create an issue and ask for help here.

@gkatsev
Copy link
Member

gkatsev commented Jan 2, 2024

When using npm, videojs doesn't get added to window by default. It'll be available on the object you require/import:

import videojs from 'video.js';

console.log(videojs.VERSION);

@Svarozic
Copy link
Contributor Author

Svarozic commented Jan 2, 2024

import videojs from 'video.js';

console.log(videojs.VERSION);

this does not work, at least package video.js@8.6.1 that I use, does not have VERSION property on function videojs imported from that package

@gkatsev
Copy link
Member

gkatsev commented Jan 2, 2024

It definitely is there and it works. Is it a type issue you're running into maybe?

See for a running example https://stackblitz.com/edit/stackblitz-starters-w9ggsk?description=React%20%20%20TypeScript%20starter%20project&file=src%2FApp.tsx,package.json&title=React%20Starter

@Svarozic
Copy link
Contributor Author

Svarozic commented Jan 2, 2024

@gkatsev it is there, thank you very much. It was a type issue on my site , this made it work (also with version 8.6.1 I work with)

import videojs from 'video.js'; 

// @ts-ignore
console.log(videojs.VERSION);                

I would consider this issue as solved.

But to solve my problem, may I somewhere ask for "new feature" , could Player instance have VERSION / version property with version of VideoJS library that was used to create that "Player component" ?

@gkatsev
Copy link
Member

gkatsev commented Jan 2, 2024

I've definitely thought that there should be a version on the player before. And we should figure out the type issue.

@Svarozic
Copy link
Contributor Author

Svarozic commented Jan 2, 2024

What would help me would be, if VideoJS library would also store library version to created instance of Player, created by videojs() function

import videojs from 'video.js';

const myPlayer = videojs(videoElement, VIDEOJS_OPTIONS);

console.log(myPlayer.version);
// or
console.log(myPlayer.VERSION); 

because in my case, my analytics library I am working on, can only access that Player instance. Is this possible or would it be a new feature request ?

@amtins
Copy link
Contributor

amtins commented Jan 2, 2024

@Svarozic shouldn't it be the role of the analytics library to ask which version of the player is being used?

Something like:

class Analytics {
  constructor(player, vjsVersion=videojs.VERSION){}
}

// OR
class AnalyticsAlternative {
  constructor(player, params, vjsVersion=videojs.VERSION)
}


// Usage
const player = videojs('player');

new Analytics(player); // will have an implicit videojs version
new AnalyticsAlternative(player, {prop1: true, prop2: 'yes'}); // will have an implicit videojs version

For comparison, here's what we do https://github.com/SRGSSR/pillarbox-web/blob/2785d8dcdefa10abc17464d53ad5479da3e48828/src/middleware/srgssr.js#L230-L235

@Svarozic
Copy link
Contributor Author

Svarozic commented Jan 2, 2024

@amtins this is something, what I am considering as possible solution to go with, to let consumer pass version as options/params property to Analytics library constructor.

But because my analytics library was detecting that version automatically before, using only simple new Analytics(videoJsPlayer); (because consumer had CDN integration of VideoJS, so my library could pick version from window.videojs.VERSION), I wanted to ask about possible "version" property in VideoJS Player instance first, to be able to have same behaviour, even when NPM integration of VideoJS is used.

The solution with default value:

class Analytics {
  constructor(player, vjsVersion=videojs.VERSION){}
}

... can not work for me, because my library is published as independent package, without video.js dependency (without import videojs from 'video.js' in Analytics class)

@amtins
Copy link
Contributor

amtins commented Jan 2, 2024

Perhaps another idea would be to provide a videojs plugin that would ensure access to the videojs version and perhaps even simplify the developer experience.

package.json

{
  "name": "analytics-plugin.js",
  "version": "1.0.0",
  "peerDependencies": {
    "video.js": ">=7",
    "analytics": ">=1"
  }
}

plugin

import videojs from 'video.js';
import Analytics from 'Analytics.js';

class AnalyticsPlugin extends videojs.getPlugin('plugin') {
  constructor(player, options) {
    super(player, options);
    this.instance = new Analytics(player, params);
  }
}

usage

import videojs from 'video.js';
import 'analytics-plugin.js';

const player = videojs('my-player', {
  plugins: {
    analyticsPlugin: {
      customerAccount: 'platinum',
      prop1: true,
      prop2: 'yes',
    },
  }
});

player.analyticsPlugin().instance.somePublicAPI()

@gkatsev
Copy link
Member

gkatsev commented Jan 2, 2024

Those are great workarounds @amtins!

I do think that having the version on the player instance would be useful. I've definitely wished for it previously when debugging in production. Since we add the player instance as a property of the player element, we can get the player instance easily in dev tools but getting videojs may not be easy if the site was built using a bundler. So, player.version() or something would be helpful.
I think it would be cool if it could work similar to how VHS's version() method works where it will include versions of loaded plugins, techs, source handlers etc., when it's able to get those versions.

@amtins
Copy link
Contributor

amtins commented Jan 2, 2024

@gkatsev player.version() looks good. 👍🏽

@Svarozic
Copy link
Contributor Author

Svarozic commented Jan 3, 2024

thank you for supporting me with my issue and for quick response, it was impressive ;)

In my case, that new feature player.version() would help me a lot @gkatsev @amtins .
Where could I follow it to see its progress and when it would be released ?

@Svarozic Svarozic closed this as completed Jan 3, 2024
@amtins
Copy link
Contributor

amtins commented Jan 3, 2024

@Svarozic could you reopen the issue, so we can link the PR to it? That way you'll be notified.

@gkatsev gkatsev reopened this Jan 3, 2024
@gkatsev gkatsev changed the title How to get VERSION from videojs installed via NPM ? feature request: expose version from player.version() Jan 3, 2024
@gkatsev
Copy link
Member

gkatsev commented Jan 3, 2024

Reopened and updated the issue title

@amtins
Copy link
Contributor

amtins commented Jan 3, 2024

@Svarozic would you like to propose a PR for this feature? That would be a great contribution! 💯

@Svarozic
Copy link
Contributor Author

Svarozic commented Jan 3, 2024

@Svarozic would you like to propose a PR for this feature?

@amtins I tried to make something quickly here #8543

@gkatsev gkatsev closed this as completed in abdfaac Jan 4, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants