Fix reloadExtension for amp-viewer-integration #24730
Conversation
src/service/extensions-impl.js
Outdated
| } | ||
| // Some extensions don't have an attribute e.g. amp-viewer-integration. | ||
| const elements = head./*OK*/ querySelectorAll( | ||
| 'script:not([custom-template]):not([custom-element])' + modifier |
There was a problem hiding this comment.
We could probably change this to `script[src*="/${extensionId}-"]` to immediately find the script with the query selector.
There was a problem hiding this comment.
Something IE supports for once.
|
https://amp.gmail.dev/playground/ is affected by this. |
Co-Authored-By: Justin Ridgewell <justin@ridgewell.name>
|
The owners check issue is a known bug fixed in ampproject/amp-github-apps#477 , which is pending review. The bugfix should be rolled out in the next day or two. |
| // Always ignore <script> elements that have a mismatched RTV. | ||
| const modifier = | ||
| ':not([i-amphtml-loaded-new-version])' + | ||
| (includeInserted ? '' : ':not([i-amphtml-inserted])'); |
There was a problem hiding this comment.
I don't understand why includeInserted is necessary. The only caller is reloadExtension? Is it trying to guard against a second call? (It won't work, the second call is guaranteed to throw because of this logic).
There was a problem hiding this comment.
Two callers:
preloadExtensionwantsincludeInserted == trueto avoid preloading inserted extensions.reloadExtensionwantsincludeInserted == falseto avoid reloading an inserted extension. The throw in this case is WAI.
There was a problem hiding this comment.
If we were to never add the :not([i-amphtml-inserted]), what effectively changes? I think both cases will continue to work.
There was a problem hiding this comment.
We can ask the person who added it. #24402 😋 Might want to keep and fix though, since we delete the extension holder on the second call and I'm not sure that's okay.
Anyways this is tangential since this PR preserves status quo. So we (you) can fix it later. :)
src/service/extensions-impl.js
Outdated
| return null; | ||
| const selectors = [ | ||
| `script[${attr}="${extensionId}"]` + modifier, | ||
| `script[src*="/${extensionId}-"]` + modifier, |
There was a problem hiding this comment.
If we want to, we can use this as the only query.
| ':not([i-amphtml-loaded-new-version])' + | ||
| (includeInserted ? '' : ':not([i-amphtml-inserted])'); | ||
| return this.win.document.head./*OK*/ querySelector( | ||
| `script[src*="/${extensionId}-"]` + modifier |
There was a problem hiding this comment.
This regex is too wide. E.g. it catches both: amp-subscriptions and amp-subscriptions-google. I'll try to go back and use an exact attribute match.
Fixes runtime error on pages that use
amp-viewer-integrationwith a mismatched RTV vs. v0.js.b/141559641