Introduce expandStringAsync and expandStringSync in url-replacement #6323
Conversation
| @@ -390,6 +390,19 @@ var forbiddenTerms = { | |||
| 'src/service/url-replacements-impl.js', | |||
| ] | |||
| }, | |||
| 'expandStringSync\\W': { | |||
There was a problem hiding this comment.
These should be in forbiddenTermsSrcInclusive. Also: \\.expandStringSync\\(
There was a problem hiding this comment.
Will do, what is the difference between the forbiddenTermsSrcInclusive and forbiddenTerms?
There was a problem hiding this comment.
Test code is allowed to use it in forbiddenTermsSrcInclusive.
| return /** @type {string} */( | ||
| this.expand_(url, opt_bindings, opt_collectVars, /* opt_sync */ true, | ||
| opt_whiteList)); | ||
| return /** @type {string} */ (this.expandUrlSync( |
| * @param {string} url | ||
| * @param {!Object<string, *>=} opt_bindings | ||
| * @return {!Promise<string>} | ||
| */ | ||
| expandAsync(url, opt_bindings) { | ||
| return /** @type {!Promise<string>} */(this.expand_(url, opt_bindings)); | ||
| return /** @type {!Promise<string>} */ ( |
| * @return {string} | ||
| */ | ||
| expandUrlSync(url, opt_bindings, opt_collectVars, opt_whiteList) { | ||
| return /** @type {string} */ ( |
| * @return {string} | ||
| */ | ||
| expandUrlSync(url, opt_bindings, opt_collectVars, opt_whiteList) { | ||
| return this.ensureProtocolMatches_(url, /** @type {string} */ (this.expand_( |
There was a problem hiding this comment.
Do we need to do the same for the whole origin?
There was a problem hiding this comment.
Not sure what do you mean, /cc @jridgewell who worked on this.
There was a problem hiding this comment.
Oh I see what you mean, basically ensure that the origin hasn't changed. @jridgewell what was the reasoning behind protecting protocol changes and does it apply to origin changes?
There was a problem hiding this comment.
Also, if you guys don't mind I'll go ahead and merge this and I opened #6338 for continuing this discussion.
|
LGTM with one question |
Follow up to #6257
Related to: #6130 and #5654