Introduce expandStringAsync and expandStringSync in url-replacement#6323
Introduce expandStringAsync and expandStringSync in url-replacement#6323mkhatib merged 2 commits intoampproject:masterfrom
Conversation
| '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.
src/service/url-replacements-impl.js
Outdated
| return /** @type {string} */( | ||
| this.expand_(url, opt_bindings, opt_collectVars, /* opt_sync */ true, | ||
| opt_whiteList)); | ||
| return /** @type {string} */ (this.expandUrlSync( |
src/service/url-replacements-impl.js
Outdated
| */ | ||
| expandAsync(url, opt_bindings) { | ||
| return /** @type {!Promise<string>} */(this.expand_(url, opt_bindings)); | ||
| return /** @type {!Promise<string>} */ ( |
src/service/url-replacements-impl.js
Outdated
| * @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