Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make esm-amd-loader dynamic require 404 test not flaky. #511

Merged
merged 4 commits into from
Jun 13, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 18 additions & 14 deletions packages/esm-amd-loader/src/esm-amd-loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ interface Window {
define: ((deps: string[], moduleBody: OnExecutedCallback) => void)&{
_reset?: () => void;
};
HTMLImports?: { importForElement: (element: Element) => HTMLLinkElement | undefined };
HTMLImports?:
{importForElement: (element: Element) => HTMLLinkElement | undefined};
}

type OnExecutedCallback = (...args: Array<{}>) => void;
Expand Down Expand Up @@ -268,9 +269,9 @@ function loadDeps(
args.push({
// We append "#<script index>" to top-level scripts so that they have
// unique keys in the registry. We don't want to see that here.
url: (module.isTopLevel === true)
? module.url.substring(0, module.url.lastIndexOf('#'))
: module.url
url: (module.isTopLevel === true) ?
module.url.substring(0, module.url.lastIndexOf('#')) :
module.url
});
continue;
}
Expand Down Expand Up @@ -555,33 +556,36 @@ function getBaseUrl(): NormalizedUrl {
}

/**
* Get the url of the current document. If the document is the main document, the base
* url is returned. Otherwise if the module was imported by a HTML import we need to
* resolve the URL relative to the HTML import.
* Get the url of the current document. If the document is the main document,
* the base url is returned. Otherwise if the module was imported by a HTML
* import we need to resolve the URL relative to the HTML import.
*
* document.currentScript does not work in IE11, but the HTML import polyfill mocks it
* when executing an import so for this case that's ok
* document.currentScript does not work in IE11, but the HTML import polyfill
* mocks it when executing an import so for this case that's ok
*/
function getDocumentUrl() {
const { currentScript } = document;
const {currentScript} = document;
// On IE11 document.currentScript is not defined when not in a HTML import
if (!currentScript) {
return baseUrl;
}

if (window.HTMLImports) {
// When the HTMLImports polyfill is active, we can take the path from the link element
// When the HTMLImports polyfill is active, we can take the path from the
// link element
const htmlImport = window.HTMLImports.importForElement(currentScript);
if (!htmlImport) {
// If there is no import for the current script, we are in the index.html. Take the base url.
// If there is no import for the current script, we are in the index.html.
// Take the base url.
return baseUrl;
}

// Return the import href
return htmlImport.href;
} else {
// On chrome's native implementation it's not possible to get a direct reference to the link element,
// create an anchor and let the browser resolve the url.
// On chrome's native implementation it's not possible to get a direct
// reference to the link element, create an anchor and let the browser
// resolve the url.
const a = currentScript.ownerDocument.createElement('a');
a.href = '';
return a.href;
Expand Down
54 changes: 35 additions & 19 deletions packages/esm-amd-loader/test/src/suite.ts
Original file line number Diff line number Diff line change
Expand Up @@ -268,19 +268,29 @@ suite('dynamic require', () => {
});

test('calls error callback only once on multiple 404s', (done) => {
let numErrors = 0;
let num404s = 0;
let numCallbackCalls = 0;

window.addEventListener('error', on404, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I don't think this is the expected behavior. require should not throw a global error, it should only call the failure callback shouldn't it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, is this the browser doing this? Emitting a window error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you can capture the browser network errors by adding the capture phase listener (the true).


function on404() {
num404s++;
if (num404s === 2) {
window.removeEventListener('error', on404);
// Need a tick to ensure the loader error handlers have fired.
setTimeout(() => {
assert.equal(numCallbackCalls, 1);
done();
});
}
}

define(['require'], (require: any) => {
require(
['./not-found-a.js', './not-found-b.js'],
() => assert.fail(),
() => numErrors++);
() => numCallbackCalls++);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, what we really want to do is wait for the first error callback, and consider the test a success if the error callback isn't called again within the next little while (i.e. using setTimeout).

That should be non-flaky.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC that would be more error prone (since network latency between the two fetches could cause the test to incorrectly pass) and slower (since we have to wait for the timeout).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did need to add a tick to let the error handlers get called though. Seems reliably green on sauce now, will see what CI says.

});

setTimeout(() => {
assert.equal(numErrors, 1);
done();
}, 1000);
});
});

Expand Down Expand Up @@ -407,16 +417,16 @@ suite('html imports', () => {
}

function testImport(href: string, expectedOrder: string[], done: () => void) {
// Each time an amd module in the chain is executed, it registers itself.
// If we've reached the length of modules we are expecing to be loaded,
// we check if the right modules were loaded in the expected order
window.addExecutedForImport = (name: string) => {
window.executionOrder.push(name);
if (window.executionOrder.length === expectedOrder.length) {
assert.deepEqual(window.executionOrder, expectedOrder);
done();
}
};
// Each time an amd module in the chain is executed, it registers itself.
// If we've reached the length of modules we are expecing to be loaded,
// we check if the right modules were loaded in the expected order
window.addExecutedForImport = (name: string) => {
window.executionOrder.push(name);
if (window.executionOrder.length === expectedOrder.length) {
assert.deepEqual(window.executionOrder, expectedOrder);
done();
}
};

importHref(href);
}
Expand All @@ -426,11 +436,17 @@ suite('html imports', () => {
});

test('modules inside deeper level html import', (done) => {
testImport('../html-import/y/deep-import.html', ['x', 'z', 'y', 'deep-import'], done);
testImport(
'../html-import/y/deep-import.html',
['x', 'z', 'y', 'deep-import'],
done);
});

test('imports with child imports', (done) => {
testImport('../html-import/parent-import.html', ['z', 'y', 'child-import', 'x', 'parent-import'], done);
testImport(
'../html-import/parent-import.html',
['z', 'y', 'child-import', 'x', 'parent-import'],
done);
});

test('import with meta', (done) => {
Expand Down