ITV-2200 Deprecate the use of *history=broadcast #527
Conversation
acarlson0000
commented
Dec 6, 2018
acarlson0000
commented
- Pass through currentUrlParams to Historian
- Update usages of hash fragment to utilize query param
- Pass through currentUrlParams to Historian - Update usages of hash fragment to utilize query param
| */ | ||
| hasBroadcastOrigin: function hasBroadcastOrigin () { | ||
| return this._historyArray.length > 0 && this._historyArray[this._historyArray.length - 1] === Historian.HISTORY_TOKEN + Historian.BROADCAST_ENTRY; | ||
| return this._urlParams.length > 0 && this._urlParams[Historian.BROADCAST_ENTRY] === true; | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
Suggest to remove the BROADCAST_ENTRY and everything to do with history= if we are moving away from this as a concept.
Then update any partner specs we have which i'm sure there were a few years ago.
Need to speak with @aevans-bbc
There was a problem hiding this comment.
Yep, this was re-factored to remove. I guess we'll need to put some tickets in for the updates to partner specs now?
- Incorrect usage of `this`, func not in scope
static/script/historian.js
Outdated
| @@ -18,9 +18,10 @@ define( | |||
| * @constructor | |||
| * @ignore | |||
| */ | |||
| init: function init (currentUrl) { | |||
| init: function init (currentUrl, UrlAppParams) { | |||
There was a problem hiding this comment.
actually - if currentUrl already contains the query parameters then no need to pass in the UrlAppParams right?
There was a problem hiding this comment.
This is true - essentially this could be achieved by currentUrl.match(/broadcast=true/) but this seemed far too hacky. Took this approach rather than a) pulling lib/application into historian b) re-implementing the queryParam extraction - but I'm happy with either way really :)
There was a problem hiding this comment.
Tiny bit of human linting...
| init: function init (currentUrl, UrlAppParams) { | |
| init: function init (currentUrl, urlAppParams) { |
static/script/historian.js
Outdated
| @@ -18,9 +18,10 @@ define( | |||
| * @constructor | |||
| * @ignore | |||
| */ | |||
| init: function init (currentUrl) { | |||
| init: function init (currentUrl, UrlAppParams) { | |||
There was a problem hiding this comment.
Tiny bit of human linting...
| init: function init (currentUrl, UrlAppParams) { | |
| init: function init (currentUrl, urlAppParams) { |
static/script/historian.js
Outdated
| var i; | ||
|
|
||
| this._urlParams = UrlAppParams; |
There was a problem hiding this comment.
| this._urlParams = UrlAppParams; | |
| this._urlParams = urlAppParams; |
Preparing to remove Historian.hasBroadcastOrigin()
Has now been replaced by application.hasBroadcastOrigin(). It is necessary to pass broadcast=true to any new URL launched via Historian.forward() if you want to propagate broadcast trust to the TAL child app.
|
After discussion, we decided we could remove the broadcast-history related stuff from Historian straight away. Awareness of whether the app has a 'broadcast origin' is now moved up a level, to Application. It would be good if you can take another look, @acarlson0000! |
Yep, agree this is much cleaner. LGTM |
