feat: ads error interface #568
Conversation
src/ads.js
Outdated
| this._error = err; | ||
|
|
||
| // TODO: log error | ||
| // do we want warn instead? Do we want to log here, or leave that up to when `error` is called? |
There was a problem hiding this comment.
I would appreciate feedback on thoughts whether we should log here or not.
There was a problem hiding this comment.
I guess it is fine to log it here
src/ads.js
Outdated
| } | ||
|
|
||
| // If `err` is null, reset the ads error. | ||
| if (err) { |
docs/integrator/api.md
Outdated
|
|
||
| To get the current ads error, the `error` function should be called with no parameters. Returns `null` if there is no error. | ||
| ```js | ||
| const currentAd = player.ads.error(); |
There was a problem hiding this comment.
| const currentAd = player.ads.error(); | |
| const currentAdError = player.ads.error(); |
docs/integrator/api.md
Outdated
|
|
||
| To set the current ads error, the `error` function should be called with a valid error. | ||
| ```js | ||
| const ad = { errorType: 'ads-error-type' } |
There was a problem hiding this comment.
| const ad = { errorType: 'ads-error-type' } | |
| const adError = { errorType: 'ads-error-type' } |
docs/integrator/api.md
Outdated
| ```js | ||
| const ad = { errorType: 'ads-error-type' } | ||
|
|
||
| player.ads.error(ad); |
There was a problem hiding this comment.
| player.ads.error(ad); | |
| player.ads.error(adError); |
package.json
Outdated
| @@ -47,7 +47,7 @@ | |||
| "node-sass": "^4.5.3", | |||
| "rollup": "1.1.0", | |||
| "sinon": "^2.2.0", | |||
| "video.js": "^8", | |||
| "video.js": "^8.11.4", | |||
There was a problem hiding this comment.
I would assume it should be updated after merging this PR: videojs/video.js#8623
src/ads.js
Outdated
| this._error = err; | ||
|
|
||
| // TODO: log error | ||
| // do we want warn instead? Do we want to log here, or leave that up to when `error` is called? |
There was a problem hiding this comment.
I guess it is fine to log it here
src/ads.js
Outdated
| * @event Player#vjsadserror | ||
| * @type {Event} | ||
| */ | ||
| player.trigger('vjsadserror'); |
There was a problem hiding this comment.
not necessary, but we can pass an error object here as event payload
src/macros.js
Outdated
| @@ -172,6 +172,11 @@ const replaceMacros = function(string, macros, uriEncode, overrides = {}) { | |||
|
|
|||
| string = string.replace(regex, uriEncodeIfNeeded(macros[macroName], uriEncode)); | |||
| } catch (error) { | |||
| player.ads.error({ | |||
| errorType: videojs.Error.AdsMacroReplacementFailed, | |||
| macro: macroName | |||
There was a problem hiding this comment.
I guess we can pass original error here
src/snapshot.js
Outdated
| @@ -196,6 +196,10 @@ export function restorePlayerSnapshot(player, callback) { | |||
| try { | |||
| resume(); | |||
| } catch (e) { | |||
| player.ads.error({ | |||
| errorType: videojs.Error.AdsResumeContentFailed | |||
There was a problem hiding this comment.
should we pass it here as well?
There was a problem hiding this comment.
Good call! I updated this in the latest commit
The implementation for an error interface specifically for ads. An error can be set on the
player.ads.error()function by passing it an object containing atypestring and ametadataobject containing any custom data that is desired. The bulk of the work is in ads.jsThere are examples of usage in this repo, but this can also be used when contrib ads is pulled into a user's repo. Errors can be handled or set by the user.
Proposed Changes: