✨ Allow Geo information in url rewriting #35558
Conversation
| }, 'AMP_GEO'); | ||
| } | ||
| ) | ||
| (geoType) => geoData(this.cachedGeo_, geoType), |
There was a problem hiding this comment.
I'm not too familiar with url-replacements, but isn't this a race condition? Is it fine to default to unknown before amp-geo service finishes loading?
There was a problem hiding this comment.
Yes, it's a race condition, but intentionally.
We opt to not interfering with delays when a user clicks on a link targetted by replacement by waiting for async data. If the data is not present for sync access here, a default fallback value is used.
Likely, if we do this, it will need some additional documentation.
There was a problem hiding this comment.
Apologies if this is a silly question, but why does it need to be sync? Why can't the URL resolve async?
There was a problem hiding this comment.
If the value is async, then users have to wait during a click event to start a navigation to the next document.
(I think we could also rewrite async items greedily as an alternative, but this isn't supported right now)
There was a problem hiding this comment.
Thank you for writing tests!
We should also update documentation in https://github.com/ampproject/amphtml/blob/main/extensions/amp-link-rewriter/amp-link-rewriter.md#output-required.
Approving to unblock / since I'm not sure if you want to address that separately
|
SGTM, I'll make the documentation change following this PR landing. |
Addresses #35482.
This isn't fully ready for merging (no tests!), but wanted feedback on the approach.Now ready for review, including a test for the sync behaviour from a cached value.
This PR will likely need additional documentation to demonstrate the inherent race condition with requesting geo information to be available for a sync action.