fix: missing CMCD params in some http requests #5072
Conversation
lib/util/cmcd_manager.js
Outdated
| this.apply_(request); | ||
| }; | ||
|
|
||
| playerInterface.getNetworkingEngine() |
There was a problem hiding this comment.
This approach has the problem that there is a publicly exported networkingEngine.clearAllRequestFilters method. If a user calls that method, expecting to remove the request filters they had added, it would be bad if it also removed our internal request filters. We haven't used request or response filters internally before this point for that reason.
We make a new CMCD manager (and this a new cmcd header filter) for each asset loaded, so I don't think it would be a common problem, but it could be an issue if someone unloads their request filters mid-playback.
One alternative I can think of (going by your rationale for this design with your comment to Joey, above, to avoid linking the two classes too tightly)...
You could add a separate array of internal-only request filters, which are used in the same way as other request filters but have a non-public register and unregister method.
There was a problem hiding this comment.
Separate internally-registered filters seems reasonable to me, assuming it's not too complex
There was a problem hiding this comment.
I've refactored the CmcdManager and NetworkingEngine.prototype.request to pass and optional context to the request call. With this info, CMCD params can be calculated directly as a RequestFilter. Additionally an onRequest handler has been added to the NetworkingEngine constructor which acts as a non-clearable request filter.
littlespex
changed the title
littlespex
changed the title
theodab
dismissed
joeyparrish’s stale review
Joey's requested changes have been fulfilled.
|
This doesn't cherry-pick cleanly to v4.3.x, so it will be out in v4.4.0. |
github-actions
bot
added
the
status: archived
Resolves #5067