Installer for extensions for dev mode #5459
Conversation
| * @const | ||
| */ | ||
| global.AMP.extension = function(name, installer) { | ||
| installer.call(null); |
There was a problem hiding this comment.
why the explicit no context?
There was a problem hiding this comment.
You mean, why not just installer()? No reason. Will switch to it.
| // Install the extension. | ||
| // DO NOT SUBMIT: First, we must implement compiler rewrite path to unwrap | ||
| // this code. | ||
| AMP.extension('amp-share-tracking', () => { |
There was a problem hiding this comment.
would it be ok to wrap the whole file with this?
There was a problem hiding this comment.
It'd be. But I'm not a big fun of that. If someone wants to unit test a specific class - there's no reason why not allow it. All I want to make sure is that we don't just install the whole thing into global window on import - that would give us proper hermetic tests.
There was a problem hiding this comment.
Ok, im trying to weight if we want to add this dynamically during dev (just through the wrapper configuration) or remove it during production build time (a bit more work but also do-able)
There was a problem hiding this comment.
I think it'd be hard to add it dynamically, unless you want to wrap the whole thing. But that's also hard. With AMP.push we first get the complete concatenated binary before wrapping. This is not the case for development.
There was a problem hiding this comment.
So, i looked around. And I think we definitely canNOT roll the whole module inside AMP.extension. Besides being an anti-pattern in our world, it'd be also very hard to do this given the multi-module code in many extensions.
There was a problem hiding this comment.
@dvoytenko do you need this ASAP? (just so i know how to prioritize)
There was a problem hiding this comment.
Was not going to submit this particular file, hence the DO NOT SUBMIT. Let's discuss offline. I'm not seeing a better way to make our tests hermetic, but we can delay this particular change and pursue a bit more manual approach.
dvoytenko
force-pushed
the
amptest1-ext
branch
from
0193c6b
to
86a3743
Compare
|
@erwinmombay PTAL |
|
@erwinmombay Changed this PR as discussed. PTAL. |
| AMP.registerElement('amp-share-tracking', AmpShareTracking); | ||
|
|
||
| // Install the extension. | ||
| AMP.extension('amp-share-tracking', AMP => { |
There was a problem hiding this comment.
is that first arg new?
i would have to to do:
(function(AMP) {
}(AMP));to mimic correctly instead of assuming global (which should also be ok)
There was a problem hiding this comment.
Yes. I realized that just deferring side effects is not enough. I also need to later install them to the right space :)
|
@erwinmombay in the meantime, is this good to be merged? |
|
@dvoytenko LGTM |
* Installer for extensions for dev mode * notes * minor * review fixes * minors * pass AMP objecy
* Installer for extensions for dev mode * notes * minor * review fixes * minors * pass AMP objecy
* Installer for extensions for dev mode * notes * minor * review fixes * minors * pass AMP objecy
No description provided.