Implement mechanism for ensuring that all extensions share an AMP version with the main binary. #7239
Conversation
…sion with the main binary. For each extension this checks the version, and if it is the wrong version, will ignore the JS file and initiate a load of the correct version. - experiment guarded. This is likely to break some stuff. - based on compiled-in version only. rtv is ignored, because we only care about code here, since config is only given in main binary. Implements locking part of ampproject#7238
| @@ -440,6 +440,7 @@ function buildExtensionJs(path, name, version, options) { | |||
| // See https://github.com/ampproject/amphtml/issues/3977 | |||
| wrapper: options.noWrapper ? '' : ('(self.AMP=self.AMP||[])' + | |||
| '.push({n:"' + name + '",' + priority + | |||
| 'v:"' + internalRuntimeVersion + '",' + | |||
There was a problem hiding this comment.
Does this include RTV prefix? I always get confused where it is added and where it's not. Does it matter?
There was a problem hiding this comment.
I have a comment at the use site for this. We care about the version of the code here, not about the version of the config.
| // This is non-obvious, but we only care about the release version, | ||
| // not about the full rtv version, because these only differ | ||
| // in the config that is fully determined by the primary binary. | ||
| if ('$internalRuntimeVersion$' == version) { |
src/runtime.js
Outdated
| // Mark the element as being replace, so that the loadExtension code | ||
| // assumes it as not-present. | ||
| scriptInHead.removeAttribute('custom-element'); | ||
| scriptInHead.setAttribute('i-amphtml-replaced-with-version', fnOrStruct.n); |
There was a problem hiding this comment.
Should it be fnOrStruct.v?
There was a problem hiding this comment.
Ping on this. Otherwise LGTM.
| if (!isExperimentOn(win, 'version-locking')) { | ||
| return false; | ||
| } | ||
| if (typeof fnOrStruct == 'function') { |
There was a problem hiding this comment.
Which extensions still use a function?
There was a problem hiding this comment.
Bunch of code that wants AMP to initialize does, not necessarily extensions. E.g. viewer integration scripts (e.g. from Google Search) do that and bunch of PWA functions.
There was a problem hiding this comment.
Basically non-AMP code.
| `[custom-element="${fnOrStruct.n}"]:not([i-amphtml-inserted])`); | ||
| dev().assert(scriptInHead, 'Expected to find script for extension: %s', | ||
| fnOrStruct.n); | ||
| if (!scriptInHead) { |
There was a problem hiding this comment.
Can you explain this one? Why wouldn't we insert one even if it didn't exist?
There was a problem hiding this comment.
There is an assertion above that makes this code unreachable in development. I think in prod it makes sense to just move forward as if nothing happened in this case.
src/runtime.js
Outdated
| // Mark the element as being replace, so that the loadExtension code | ||
| // assumes it as not-present. | ||
| scriptInHead.removeAttribute('custom-element'); | ||
| scriptInHead.setAttribute('i-amphtml-replaced-with-version', fnOrStruct.n); |
There was a problem hiding this comment.
Sorry, tried a new name for the attribute :)
I want the name here for easier traceability of the DOM.
| * @return {boolean} | ||
| */ | ||
| function maybeLoadCorrectVersion(win, fnOrStruct) { | ||
| if (!isExperimentOn(win, 'version-locking')) { |
There was a problem hiding this comment.
Do you want to do instead isExperimentOnAllowUrlOverride?
There was a problem hiding this comment.
Could you explain why you think that makes sense?
…sion with the main binary. (ampproject#7239) For each extension this checks the version, and if it is the wrong version, will ignore the JS file and initiate a load of the correct version. - experiment guarded. This is likely to break some stuff. - based on compiled-in version only. rtv is ignored, because we only care about code here, since config is only given in main binary. Implements locking part of ampproject#7238
…sion with the main binary. (ampproject#7239) For each extension this checks the version, and if it is the wrong version, will ignore the JS file and initiate a load of the correct version. - experiment guarded. This is likely to break some stuff. - based on compiled-in version only. rtv is ignored, because we only care about code here, since config is only given in main binary. Implements locking part of ampproject#7238
For each extension this checks the version, and if it is the wrong version, will ignore the JS file and initiate a load of the correct version.
Implements locking part of #7238