Skip to content

Commit

Permalink
Allow reloadExtension() to soft fail (#29042)
Browse files Browse the repository at this point in the history
* Allow reloadExtension() to soft fail.

* No one uses the return value.

* Actually one test uses the return value.

* s/warn/error
  • Loading branch information
William Chou committed Jun 26, 2020
1 parent 90702eb commit 46def58
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 19 deletions.
18 changes: 11 additions & 7 deletions src/service/extensions-impl.js
Expand Up @@ -20,7 +20,7 @@ import {
calculateExtensionScriptUrl,
parseExtensionUrl,
} from './extension-location';
import {dev, devAssert, rethrowAsync} from '../log';
import {dev, devAssert, rethrowAsync, user} from '../log';
import {getMode} from '../mode';
import {installStylesForDoc} from '../style-installer';
import {map} from '../utils/object';
Expand Down Expand Up @@ -239,19 +239,23 @@ export class Extensions {
/**
* Reloads the new version of the extension.
* @param {string} extensionId
* @return {!Promise<!ExtensionDef>}
* @return {?Promise<!ExtensionDef>}
*/
reloadExtension(extensionId) {
// Ignore inserted script elements to prevent recursion.
const els = this.getExtensionScripts_(
extensionId,
/* includeInserted */ false
);
devAssert(
els.length > 0,
'Cannot find script for extension: %s',
extensionId
);
if (!els.length) {
const TAG = 'reloadExtension';
user().error(
TAG,
'Extension script for "%s" is missing or was already reloaded.',
extensionId
);
return null;
}
// The previously awaited extension loader must not have finished or
// failed.
const holder = this.extensions_[extensionId];
Expand Down
38 changes: 26 additions & 12 deletions test/unit/test-extensions.js
Expand Up @@ -21,6 +21,7 @@ import {Extensions} from '../../src/service/extensions-impl';
import {Services} from '../../src/services';
import {getServiceForDoc} from '../../src/service';
import {installTimerService} from '../../src/service/timer-impl';
import {user} from '../../src/log';

class AmpTest extends BaseElement {}
class AmpTestSub extends BaseElement {}
Expand Down Expand Up @@ -643,14 +644,18 @@ describes.sandboxed('Extensions', {}, () => {
env.sandbox.stub(Services, 'ampdocServiceFor').returns(null);
extensions = new Extensions(win);
env.sandbox.stub(extensions, 'preloadExtension');
env.sandbox.stub(user(), 'error');
});

describe('regular scripts', () => {
it('should devAssert if script cannot be found', () => {
expect(() => {
allowConsoleError(() => extensions.reloadExtension('amp-list'));
}).to.throw('Cannot find script for extension: amp-list');
extensions.reloadExtension('amp-list');

expect(user().error).to.be.calledWith(
'reloadExtension',
'Extension script for "%s" is missing or was already reloaded.',
'amp-list'
);
expect(extensions.preloadExtension).to.not.be.called;
});

Expand All @@ -664,10 +669,13 @@ describes.sandboxed('Extensions', {}, () => {
list.setAttribute('i-amphtml-inserted', '');
win.document.head.appendChild(list);

expect(() => {
allowConsoleError(() => extensions.reloadExtension('amp-list'));
}).to.throw('Cannot find script for extension: amp-list');
extensions.reloadExtension('amp-list');

expect(user().error).to.be.calledWith(
'reloadExtension',
'Extension script for "%s" is missing or was already reloaded.',
'amp-list'
);
expect(list.hasAttribute('i-amphtml-loaded-new-version')).to.be.false;
expect(extensions.preloadExtension).to.not.be.called;
});
Expand Down Expand Up @@ -751,10 +759,13 @@ describes.sandboxed('Extensions', {}, () => {

describe('module/nomdule script pairs', () => {
it('should devAssert if script cannot be found', () => {
expect(() => {
allowConsoleError(() => extensions.reloadExtension('amp-list'));
}).to.throw('Cannot find script for extension: amp-list');
extensions.reloadExtension('amp-list');

expect(user().error).to.be.calledWith(
'reloadExtension',
'Extension script for "%s" is missing or was already reloaded.',
'amp-list'
);
expect(extensions.preloadExtension).to.not.be.called;
});

Expand All @@ -779,10 +790,13 @@ describes.sandboxed('Extensions', {}, () => {
nomod.setAttribute('nomodule', '');
win.document.head.appendChild(nomod);

expect(() => {
allowConsoleError(() => extensions.reloadExtension('amp-list'));
}).to.throw('Cannot find script for extension: amp-list');
extensions.reloadExtension('amp-list');

expect(user().error).to.be.calledWith(
'reloadExtension',
'Extension script for "%s" is missing or was already reloaded.',
'amp-list'
);
expect(mod.hasAttribute('i-amphtml-loaded-new-version')).to.be.false;
expect(nomod.hasAttribute('i-amphtml-loaded-new-version')).to.be.false;
expect(extensions.preloadExtension).to.not.be.called;
Expand Down

0 comments on commit 46def58

Please sign in to comment.