Add amp-story URL replacement support#11528
Conversation
c969bc0 to
0d059eb
Compare
dreamofabear
left a comment
There was a problem hiding this comment.
Ah, looks like a lot of other extension-specific code is here too. 😝
| } | ||
|
|
||
| /** | ||
| * @param {!Window} win |
| /** @private {?Promise<?ShareTrackingFragmentsDef>} */ | ||
| this.shareTrackingFragments_ = null; | ||
|
|
||
| /** @private {?Promise<?{pageIndex: number, pageId: string}>} */ |
There was a problem hiding this comment.
This should return AmpStoryVariableService right?
There was a problem hiding this comment.
That would be ideal, but I'd rather not add the whole amp-story tree to the general source file set (otherwise the module can't be resolved).
There was a problem hiding this comment.
We're not really retrieving an element "service" here though. How are we registering that data as a service with the runtime? registerService?
There was a problem hiding this comment.
There was a problem hiding this comment.
Ohh I see. 😃 I think adding the extension to the Closure file path is fine. Erwin's working on making Closure compile runtime + extensions in a single pass anyways. Replacing a class with a record-type of its getter property interface confused me a lot.
There was a problem hiding this comment.
This is super confusing. Why is it a record? Don't these change? There are a lot of services here that function exactly same way, e.g. getAccessService_.
This definitely needs to change. Please send a PR.
| getStoryValue_(getter, expr) { | ||
| if (!this.storyVariables_) { | ||
| this.storyVariables_ = | ||
| Services.storyVariableServiceForOrNull(this.ampdoc.win); |
There was a problem hiding this comment.
This could return null again (if the AMP page doesn't have the story extension installed), which means calling .then() on it will cause an error. In that case, let's throw a user error asking them to install amp-story.
There was a problem hiding this comment.
- Re: error. I think the convention in this case is just to assert the error on promise resolution.
- Re: null-value, I don't think that's the case.
storyVariableServiceForOrNullwill return a promise that resolves to null, not null itself.
See getVairiantsValue_ (sic), getShareTrackingValue_ for prior art on the same use-case (i.e. reading extension-specific values).
There was a problem hiding this comment.
Oh, my mistake. Can we change this.storyVariables_ to this.storyVariablesPromise_?
| getStoryValue_(getter, expr) { | ||
| if (!this.storyVariables_) { | ||
| this.storyVariables_ = | ||
| Services.storyVariableServiceForOrNull(this.ampdoc.win); |
There was a problem hiding this comment.
Oh, my mistake. Can we change this.storyVariables_ to this.storyVariablesPromise_?
This adds support for the rewriting the
STORY_PAGE_INDEXandSTORY_PAGE_IDvariables for analytics./cc @newmuis