Skip to content
Merged
Changes from 1 commit
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
45 changes: 40 additions & 5 deletions packages/esm-amd-loader/src/esm-amd-loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ interface Window {
define: ((deps: string[], moduleBody: OnExecutedCallback) => void)&{
_reset?: () => void;
};
HTMLImports: { importForElement: (element: Element) => HTMLLinkElement | undefined }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

HTMLImports?: ... since it might not be defined

}

type OnExecutedCallback = (...args: Array<{}>) => void;
Expand Down Expand Up @@ -421,14 +422,15 @@ window.define = function(deps: string[], moduleBody?: OnExecutedCallback) {
return [deps, moduleBody];
};

// Case #2: We are a top-level script in the HTML document. Our URL is the
// document's base URL. We can discover this case by waiting a tick, and if
// we haven't already been defined by the "onload" handler from case #1,
// then this must be case #2.
// Case #2: We are a top-level script in the HTML document or a HTML import.
// Resolve the URL relative to the document url. We can discover this case
// by waiting a tick, and if we haven't already been defined by the "onload"
// handler from case #1, then this must be case #2.
const documentUrl = getDocumentUrl();
setTimeout(() => {
if (defined === false) {
pendingDefine = undefined;
const url = baseUrl + '#' + topLevelScriptIdx++ as NormalizedUrl;
const url = documentUrl + '#' + topLevelScriptIdx++ as NormalizedUrl;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you'll also need to update line 267 (url: (module.isTopLevel === true) ? baseUrl : module.url), because when a module is top-level, we currently just return the const document URL when meta is requested. To do it correctly you'll need to return this new module.url with the artificial # fragment we added stripped off.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point, I've made the change.

// It's actually Initialized, but we're skipping over the Loading
// state, because this is a top level document and it's already loaded.
const mod = getModule(url) as ModuleG<StateEnum.Loading>;
Expand Down Expand Up @@ -545,4 +547,37 @@ function getBaseUrl(): NormalizedUrl {
(document.querySelector('base') || window.location).href) as
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.
*
* 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;
// 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
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.
return baseUrl;
}
// Take the import href and strip off the filename
return htmlImport.href.substring(0, htmlImport.href.lastIndexOf('/') + 1);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we need to be a bit more careful here, e.g. this will return http:// if href is http://example.com. We handle this above with normalizeUrl (which ensures there is always a trailing slash), and getUrlBase (which handles some more cases).

@LarsDenBakker LarsDenBakker May 30, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if the href will ever be http://example.com for a html import, won't it always need to be a specific file? But it turns out we can just return the href here, this is in line with what we're returning from the native implementation as well.

} else {
// On chrome's native implementation it's not possible to get a direct reference to the link element,
// create a new script and let the browser resolve the url.
const script = currentScript.ownerDocument.createElement('script');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd use an a tag with an href instead of a script, just to be more consistent with what we do with normalizeUrl above.

script.src = './';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this needs to be '' instead of './', otherwise you'll get e.g. http://localhost/ instead of http://localhost/import.html.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I didn't think '' would resolve to anything, but it did. It's better because it resolves to the filename.

return script.src;
}
}
})();