Amphtml: Get rid of the side effect of importing amp-abc.js

[lannka]

import {AmpAnalytics} from '../amp-analytics';

The above line in tests would have a side effect causing amp-analytics custom element registered on global window. We should consider split all extension js to

in amp-analytics/0.1/amp-analytics.js

export class AmpAnalytics {
  ...
}

in amp-analytics/0.1/main.js

AMP.extension(TAG, '0.1', AMP => {
  // Register doc-service factory.
  AMP.registerServiceForDoc(
    'amp-analytics-instrumentation',
    InstrumentationService
  );
  AMP.registerServiceForDoc('activity', Activity);
  installLinkerReaderService(AMP.win);
  AMP.registerServiceForDoc('amp-analytics-variables', VariableService);
  // Register the element.
  AMP.registerElement(TAG, AmpAnalytics);
});

No unit test should ever import main.js, amp.js etc.

[rsimha]

@lannka Some preliminary results below.

1. I moved the AMP.extension() call for amp-youtube.js into a new main.js in extensions/amp-youtube/0.1/
2. This initially caused all unit tests for amp-youtube to fail, because the extension is never installed
3. Importing main.js from test-amp-youtube.js defeats the purpose, so I didn't do that
4. I added a before() section to test-amp-youtube.js where I called AMP.extension() to initialize the extension before running tests (this duplicates a fair bit of code)

This might look promising, but it doesn't fully eliminate the need to call adopt() on the global window in _init_tests.js. This is because amp-youtube.js will still be imported before Mocha runs its global before() function.

We still need to establish things like global.AMP, AMP.BaseElement, etc. before amp-youtube.js can be imported, and currently, the only way to do this is via a global adopt() in _init_tests.js.

This is what I have so far. Let me know if you have other ideas while I continue to investigate.

/cc @cramforce @choumx @jridgewell @erwinmombay for runtime expertise

See related issue #23837

[rsimha]

Just chatted with @jridgewell and @choumx about migrating all existing tests that use a top-level describe to using describes.realWin. I found about ~200 such instances across ~150 test files, so it should be possible to do this in a refactor.

This should pave the way for making extension imports free of global side effects at the infrastructure level.

[rsimha]

In https://github.com/ampproject/amphtml/issues/23839#issuecomment-521411580, I took a stab at changing the way extensions are initialized (to make importing a .js file side-effect free), but the effort didn't yield much fruit.

Meanwhile, in #24090, a rule was added to forbid top-level describe blocks. However, there are ~150 legacy tests that are yet to be migrated:

https://github.com/ampproject/amphtml/blob/5d94316dd702748ec4aa3c16eca060f6deaa04bd/build-system/tasks/presubmit-checks.js#L756-L894

Curious about whether the folks already cc'd on this issue have additional thoughts on this issue, specifically making extension imports side-effect free.

Edit: /cc @dvoytenko, who suggested that importing extensions is already side-effect free

[dvoytenko]

Hmm. All extensions should already be side-effect free via AMP.extension(). Here's how it's handled in the test:

https://github.com/ampproject/amphtml/blob/72e82c633e46a301ec04b7b5824f59af778ec460/test/_init_tests.js#L77

[rsimha]

Makes sense, thanks Dima. Closing out this issue.

[dvoytenko]

That's the intention, but perhaps this is broken in some cases? Or maybe some extensions are missing AMP.extension()?

[rsimha]

That's the intention, but perhaps this is broken in some cases? Or maybe some extensions are missing AMP.extension()?

Pretty sure this isn't the case, because every AMP.extension() call contains AMP.registerElement(), without which I'm not sure the extension will even work.

/cc @lannka for comment, in case I'm missing something.

[dvoytenko]

@rsimha The way it works is this with tests:

1. All AMP.extension(component, version, installer) callbacks are stored in an internal buffer map, e.g. {'amp-ext1:0.1', installer}. They are not executed. So AMP.registerElement() will not be called until later.
2. A test can specify the extensions: ['amp-ext:0.1'] in its configuration. The test runner (describes fixture) will go through a set of extensions specified this way, look up the installers in the buffer map that we created on step 1, and calls the callback with the right test window/ampdoc.

Without some test fixture calling the installer at some point, it should never get called by anyone else.