Create url-replacement for embeds. #6133
Conversation
avimehta
force-pushed
the
a4a2
branch
2 times, most recently
from
7c833a4
to
f31b399
Compare
avimehta
changed the title
| export class A4AVariableSource extends VariableSource { | ||
| constructor(ampdoc, window) { | ||
| super(); | ||
| this.globalVariableSource_ = urlReplacementsForDoc(ampdoc) |
| super(); | ||
| this.globalVariableSource_ = urlReplacementsForDoc(ampdoc) | ||
| .getVariableSource(); | ||
| this.win_ = window; |
|
|
||
| /** Provides A4A specific variable substitution. */ | ||
| export class A4AVariableSource extends VariableSource { | ||
| constructor(ampdoc, window) { |
| return getNavigationData(this.win_, 'redirectCount'); | ||
| }); | ||
|
|
||
| for (let v = 0; v < WHITELISTED_VARIABLES.length; v++) { |
There was a problem hiding this comment.
Hmm. I'm generally more in preference with the runtime model, e.g. resolveVar that uses globalResolver.resolveVar. But let's see how this works out.
There was a problem hiding this comment.
That is what I was initially doing.
Kind of difficult without doing a major refactoring. Currently, you need to call set() for each variable otherwise, getExpr() will not include the variable in regex and hence, the variable-source will never be called for that particular varName. As a result, substitutions never happen for variables for which set is not called.
There was a problem hiding this comment.
Cool. Let's keep going with this.
| for (let v = 0; v < WHITELISTED_VARIABLES.length; v++) { | ||
| const varName = WHITELISTED_VARIABLES[v]; | ||
| const resolvers = this.globalVariableSource_.getReplacement(varName); | ||
| this.set(varName, resolvers.sync).setAsync(varName, resolvers.async); |
There was a problem hiding this comment.
At list, maybe we can have a single setBoth?
|
|
||
| /** Provides A4A specific variable substitution. */ | ||
| export class A4AVariableSource extends VariableSource { | ||
| constructor(ampdoc, window) { |
There was a problem hiding this comment.
Let's rename window to embedWin
| return this.getTimingDataAsync_(startEvent, endEvent); | ||
| }); | ||
| /** @private {boolean} */ | ||
| this.initialized_ = false; |
There was a problem hiding this comment.
Where is this used and how?
| @@ -745,7 +634,7 @@ export class UrlReplacements { | |||
| let binding; | |||
| if (opt_bindings && (name in opt_bindings)) { | |||
| binding = opt_bindings[name]; | |||
| } else if ((binding = this.getReplacement_(name))) { | |||
| } else if ((binding = this.variableSource_.getReplacement(name))) { | |||
There was a problem hiding this comment.
Let's call this method simply as get
| * @param {!Window} embedWin | ||
| * @param {*} varSource | ||
| */ | ||
| export function installUrlReplacementsForEmbed(ampdoc, embedWin, varSource) { |
There was a problem hiding this comment.
This method should go to the url-replacements-impl. Why? To avoid any risk of all other extensions compiling the whole UrlReplacements in. Only whoever needs it will compile it by depending on url-replacements-impl directly. Does it make sense?
There was a problem hiding this comment.
That is why I added the dep-check. Does that not fix it?
There was a problem hiding this comment.
But there's no deep-check on url-replacements.js which is imported by many many extensions. Generally, all installs should go to -impl.js files, because there's no way to install the implementation unless you compile that file in anyway.
There was a problem hiding this comment.
i see. thanks for the answer.
| @@ -0,0 +1,235 @@ | |||
| /** | |||
There was a problem hiding this comment.
This file should go to ./service/ dir
There was a problem hiding this comment.
why so? This isn't a service now.
There was a problem hiding this comment.
Hmm. Maybe you are right. I was mostly reacting that it's only used by the service or service-depending code. But we can keep it here for now.
There was a problem hiding this comment.
i moved it :(. Should I keep it in src/service or move back to src/ ?
There was a problem hiding this comment.
No. Let's keep in ./service/ :)
Tests are pending. For ampproject#5657.
|
@cramforce would you like to review this? @dvoytenko any other tests you want me to add. Once case I can think of is for test-amp-a4a.js which ensures that new url-replacements.js is present. It's not really a unittest and I am trying to figure out how to best do it. |
|
@avimehta For that unit test, we simply need to call |
|
done. I think I addressed all comments :) |
| installServiceInEmbedScope(embedWin, 'url-replace', {varSource}); | ||
| export function installUrlReplacementsForEmbed(ampdoc, embedWin, varSource) { | ||
| installServiceInEmbedScope(embedWin, 'url-replace', | ||
| new UrlReplacements(ampdoc, varSource)); |
There was a problem hiding this comment.
done. Merging the PR now.
* Create url-replacement for embeds. For ampproject#5657.
* Create url-replacement for embeds. For ampproject#5657.
* Create url-replacement for embeds. For ampproject#5657.
For #5657.