feat: on license request errors, return response body as cause

[gkatsev]

This adds a new cause property to the error object returned by player.error() when a license request fails with a 4xx or 5xx status code. It's a string representation, so, if it's JSON a consumer would need to call JSON.parse manually as we can't know whether it's JSON or not.

One way of using it is by listening to the error event and then re-setting the message to be the cause

// notice the .one here, otherwise we'd get caught in an infinite loop
player.one('error', () => {
  var e = player.error();
  player.error(null);
  player.error({code: 5, message: e.cause})
});

[gkatsev]

In addition to the comments below, are there any other places where we may need to bubble the cause through?

[gkatsev]

fairplay certificate 4xx and 5xx were never counted as errors apparently.

[brandonocasey]

should we wrap this in a checkForError function and re-use it?

[brandonocasey]

or perhaps emeErrorHandler should take the response and trigger an error on error, and return true so that we can return.

[gkatsev]

Yeah, maybe we can push it into emeErrorHandler, though, I'd need to figure out whether the custom getLicense/getCertificate method's callback get to emeErrorHandler or how that piece is wired up.

Also, is it possible that treating 4xx/5xx for these as errors now going to cause an issue compared to previously? My guess is likely no, since if we didn't actually get the cert, it means that playback didn't work.

[gkatsev]

pushing it all into emeErrorHandler might make things more complicated because of existing and custom getLicenses, which expect it to be just err, and license. Though, we could use the function arity to disambiguate between the two, where if we only get 2 arguments, it's the existing stuff, and if it gets 3 items, it's the new thing since we can just pass through the err, res, resBody. I'll take a look to see whether it's worth it.

[gkatsev]

I think it makes it a bit clearer what's happening, plus allows us to set both the cause and the message based on the potential inputs to this function.

[gkatsev]

These were incorrect. As we specify that the response type should be an arraybuffer, we need to convert these to array buffer.

[brandonocasey]

I do wonder wether we should trigger a beforeerror event that can modify the error object before we trigger it.

[gkatsev]

Re: beforeerror, I think that adding a beforeerror hook might nice, but that's out of scope for right now.

[gkatsev]

Thinking about things some more, I don't think that for custom getLicense requests we can properly provide helpers, particularly, since fetch exists. I think the current err and license arguments make sense. Instead, we should update our documentation to mention that you should pass an error object when the HTTP Status Code is 4xx or 5xx and that you can provide the response body in the cause field for downstream consumers of this error. Right now, our sample documentation only shows providing an error if the XHR errored itself, which rarely happens.

[gkatsev]

However, internally, I think we can use a helper so that all this error stuff is handled the same across all our usage instead of trying to remember to copy-pasta it all over the place.

[brandonocasey]

Does any of this throw if responseBody is empty?

[brandonocasey]

Might be worth adding some tests for these two functions.

[gkatsev]

Yup, definitely need a bunch of tests before this is ready.
Looks like decoding nothing doesn't throw.

> var td = new TextDecoder('utf-8')
> td.decode()
< ""

[gkatsev]

Moving to videojs/xhr#7, I did end up wrapping this in a try catch just in case.

[brandonocasey]

Once we switch this code to use videojs.xhr I think this is good to go.

[gkatsev]

This will require a Video.js release with videojs/video.js#7348 which includes videojs/xhr#7.