-
Notifications
You must be signed in to change notification settings - Fork 3
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/conviva device metadata #63
Conversation
🦋 Changeset detectedLatest commit: 6a2caf5 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
conviva/package.json
Outdated
@@ -21,7 +21,7 @@ | |||
"bundle": "rollup -c rollup.config.mjs", | |||
"watch": "npm run bundle -- --watch", | |||
"build": "npm run clean && npm run bundle", | |||
"serve": "http-server", | |||
"serve": "http-server ./.. -o /conviva/test/pages/main_umd.html", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to open the pages folder instead? Then they can choose our other pages more easily if they wanna test yospace for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
conviva/test/pages/main_umd.html
Outdated
@@ -31,7 +31,20 @@ | |||
const convivaConfig = { | |||
debug: false, | |||
gatewayUrl: 'CUSTOMER_GATEWAY_GOES_HERE', | |||
customerKey: 'CUSTOMER_KEY_GOES_HERE' // Can be a test or production key. | |||
customerKey: 'CUSTOMER_KEY_GOES_HERE', // Can be a test or production key. | |||
deviceMetadata: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to have this specific device metadata on our test page while we can't know the users setup? It is already mentioned in the README that it is an option to set this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
@@ -327,7 +329,9 @@ export class ConvivaHandler { | |||
private readonly onSourceChange = () => { | |||
this.maybeReportPlaybackEnded(); | |||
this.currentSource = this.player.source; | |||
this.customMetadata = {}; | |||
if (this.currentSource === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for this check? Why not always reset it on source change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because some customers provide the content metadata through the constructor and set the corresponding source immediately after. Which, based on the README, is a valid way to do it. However, right now setting a source in the player has the unwanted side effect of clearing the customMetadata.
What would be your preferred fix for this?
- update the readme and say users should always use the
setContentInfo
method after setting a source - not clear
this.customMetadata
at all onsourcechange
- stick with the current fix
I actually prefer option (2) now 😇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC the idea behind resetting customMetadata
on source change is that a lot of the ConvivaMetadata
can be source specific such as asset name. Some parts can be shared across sources sure but that is not guaranteed. There is also no way to reset customMetadata
from outside of the player atm.
The metadata passed in the constructor is actually used as customMetadata
, but also convivaMetadata
, so everything passed in the constructor should be handled as shared metadata for the whole player session, while customMetadata can be updated for each source and thus reset on source change.
Now as you mention setting customMetadata
in the constructor might not make much sense as customers can first create the connector and then set their sources. convivaMetadata
is always passed to conviva at reportPlaybackRequested
. I think this has the same effect as setContentInfo
so the duplication might not be necessary but maybe should double check this.
If my assumption above is correct, then I would prefer to update the README and make it clear that the metadata passed in the constructor is kept for the whole connector lifetime and customMetadata
is source specific.
Of course I'm open to other ideas in case this is not sufficient🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some parts can be shared across sources
I think only PLAYER_NAME
and VIEWER_ID
can be considered shareable across sources. That said, I understand customers thinking customMetadata
and convivaMetadata
can be treated the same way since they initially contain the same data and have the same type.
everything passed in the constructor should be handled as shared metadata for the whole player session
I'd say this is not the case now. The only guarantee is indeed that it's passed to reportPlaybackRequested
and to be sure it's used throughout the session, the customer needs to set it again with setContentInfo
.
There is also no way to reset customMetadata from outside of the player atm.
I like stopAndStartNewSession
because you're being very explicit about what needs to / is expected to happen. If we need an API to reset customMetadata
, we can add it and let users be explicit about this instead of having it as a side effect of an interaction with the player.
customMetadata
can be updated for each source and thus reset onsourcechange
Either the customMetadata
is outdated or it's missing after a sourcechange
. Action from the customer is required either way, so it doesn't hurt to leave it there.
Note that resetting customMetadata
on sourcechange
was introduced in #35
If the above isn't convincing enough, then I propose to add this to the README:
Note that the
convivaMetadata
provided to theConvivaConnector
constructor is primarily used to pass on to the Conviva SDK'sreportPlaybackRequested
. If a source is set to the player after initialisation the connector, you should always provide the corresponding metadata (again) through the connector'ssetContentInfo
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I verified that the metadata passed to the constructor is immediately set for each new session, meaning that metadata passed to reportPlaybackRequested
and setContentInfo
are both reported in a similar fashion. So maybe customMetadata
can be removed from the constructor and only updated through setContentInfo
.
I think only PLAYER_NAME and VIEWER_ID can be considered shareable across sources.
Customers can add as many custom tags as they want but I assume that most of the metadata is indeed source specific as you mention. So this makes it logical for me to reset the customMetadata
on a sourcechange
event.
Either the customMetadata is outdated or it's missing after a sourcechange. Action from the customer is required either way, so it doesn't hurt to leave it there.
Action from the customer is required sure but if we know that most of the data won't be valid anymore why should we keep it? My opinion is that it is better and more noticeable for the customer if no data is present than if incorrect data is reported. Though this should indeed be mentioned clearly in the README to not cause any confusion.
I am fine of course with adding a way to reset the customMetadata
on top of this to not only rely on sourcechange
.
This PR adds an optional property
deviceMetadata
to the ConvivaConfiguration. This allows users to pass the desired device metadata to the Conviva SDK.It also includes a fix for an issue where te asset name was reported as
"NA"
after a few seconds. This could happen for users that just pass the content metadata only through theConvivaConnector
constructor and set the player source right after that.