Get document URL in esm-amd-loader#407

Merged

aomarks merged 20 commits intoPolymer:masterfrom

LarsDenBakker:feature/resolve-html-import-url

Jun 7, 2018

Contributor

LarsDenBakker commented May 23, 2018 •

edited

Loading

Currently the esm-amd-loader assumes that when it finds a top level module script, it's always a script in the index.html. When using HTML Imports (and possibility in the future HTML modules as well) this is not always the case.

Relative module import specified in HTML imports are resolved properly for es modules, but when transpiled to AMD they are always resolved relative to the index.html.

This PR makes it so that relative imports are resolved relative to the document (index or HTML import) that includes the script. It also makes it so that the correct import.meta.url is returned for these AMD modules.

I didn't change the tests yet, I first wanted to see if this was the right direction to fix HTML import - es module interop.

Fixes #391


          Get document URL in esm-amd-loader

0c68b63

googlebot added the cla: yes label

aomarks requested changes

View reviewed changes

Member

aomarks left a comment

Thanks for the PR! Looks good overall.

packages/esm-amd-loader/src/esm-amd-loader.ts

                   if (defined === false) {
                     pendingDefine = undefined;
-                    const url = baseUrl + '#' + topLevelScriptIdx++ as NormalizedUrl;
+                    const url = documentUrl + '#' + topLevelScriptIdx++ as NormalizedUrl;

Member

aomarks May 29, 2018

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.

Contributor Author

LarsDenBakker May 30, 2018

Good point, I've made the change.

packages/esm-amd-loader/src/esm-amd-loader.ts Outdated

                 define: ((deps: string[], moduleBody: OnExecutedCallback) => void)&{
                   _reset?: () => void;
                 };
+                HTMLImports: { importForElement: (element: Element) => HTMLLinkElement | undefined }

Member

aomarks May 29, 2018

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

packages/esm-amd-loader/src/esm-amd-loader.ts Outdated

+                } 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');

Member

aomarks May 29, 2018

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.

packages/esm-amd-loader/src/esm-amd-loader.ts Outdated

+                  // 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');
+                  script.src = './';

Member

aomarks May 29, 2018

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

Contributor Author

LarsDenBakker May 30, 2018

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

packages/esm-amd-loader/src/esm-amd-loader.ts Outdated

+                    return baseUrl;
+                  }
+                  // Take the import href and strip off the filename
+                  return htmlImport.href.substring(0, htmlImport.href.lastIndexOf('/') + 1);

Member

aomarks May 29, 2018

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).

Contributor Author

LarsDenBakker May 30, 2018 •

edited

Loading

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.

Lars den Bakker added 10 commits

May 30, 2018 09:08


          Fix HTMLImports typing

9adc331


          Add webcomponentsjs dependency

b1d7def


          Added tests for amd loader and html imports

4f2d5b8


          Reference the right method

790de98


          Return the right import.meta.url for modules in html imports

e8f136f


          Use an anchor instead of a script for consistency

8c1e6fd


          On native html imports, resolve anchor to '' instead of './'

14b8fae


          Don't strip off the filename for polyfilled html imports

7e59cca


          Add webcomponents v1 polyfill

95c92ad


          Use generic import function

a0f2f83

Contributor Author

LarsDenBakker commented May 30, 2018 •

edited

Loading

@aomarks I've made the changes and added tests.

I couldn't get the tests to run from a html import, wct doesn't run suite() and test() created inside an import. I didn't want to introduce too much deviation from the current set up just for html imports. Creating the imports imperatively does the job pretty simply as well and keeps it in the same file. What do you think?

I added the v1 webcomponents loader, as the v2 loader does not add html imports.

Lars den Bakker added 2 commits

May 30, 2018 22:14


          Merge branch 'master' of https://github.com/Polymer/tools into featur…

4a2f82e

…e/resolve-html-import-url


          Update goldens

74c41c4

Contributor Author

LarsDenBakker commented Jun 2, 2018

The tests run fine for me locally, but I'm not sure what's going wrong inside appveyor and travis?


          Add test for modules importing other modules

f23ac6f

Member

aomarks commented Jun 7, 2018

Looks good, just a few small more comments. Sorry for the delay! I'm also working on #441 so that we'll have this library running on Safari and IE11 in continuous integration. Have you tested manually on IE11 btw?

aomarks requested changes

View reviewed changes

packages/esm-amd-loader/test/src/suite.ts Outdated

+                }
+                function testImport(href: string, expectedOrder: string[], done: () => void) {
+                    // Each time and amd module in the chain is executed, it registers itself.

Member

aomarks Jun 7, 2018

s/and/an

Contributor Author

LarsDenBakker Jun 7, 2018

Done

packages/esm-amd-loader/test/src/suite.ts Outdated

+                    // Each time and 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.addExcutedForImport = (name: string) => {

Member

aomarks Jun 7, 2018

addExecutedForImport

Contributor Author

LarsDenBakker Jun 7, 2018

Done

packages/esm-amd-loader/tsconfig.json Outdated

                   "sourceMap": false,
                   "lib": [
-                    "es5",
+                    "es2015",

Member

aomarks Jun 7, 2018

Do you need this to be es2015? We need the tests to run in IE11 without adding polyfills so only having es5 helps make sure we aren't using APIs that won't be available.

Contributor Author

LarsDenBakker Jun 7, 2018

Sorry, I didn't mean to change that.

packages/esm-amd-loader/test/src/suite.ts Outdated

+                test('import with meta', (done) => {
+                  window.testImportMeta = (url) => {
+                      assert.match(url, /https?:\/\/.+\/html-import\/meta/);

Member

aomarks Jun 7, 2018

check that it ends exactly in html-import/meta/import-meta.html$

Contributor Author

LarsDenBakker Jun 7, 2018

You're right, made the change.

packages/esm-amd-loader/test/static/html-import/meta/import-meta.html

+              <script>
+                define(['meta'], (meta) => {
+                  window.testImportMeta(meta.url);

Member

aomarks Jun 7, 2018

Might be a little nicer to have this set a global, and then check it inside testImport before you call done (or something like that) to ensure that the assertion is actually run. Right now if this module didn't execute at all for some reason, the test would still pass.

Contributor Author

LarsDenBakker Jun 7, 2018

I think this is already the case. In the start of the test we make a global function which is called from the import. If the module file fails to load or if the code fails to run, the function isn't called and thus done isn't called. This causes the test to fail.

Lars den Bakker added 4 commits

June 7, 2018 08:19


          Transpile to es5 again

ae21038


          Fix typos

5a7679a


          Update test to check if import ends with filename

c7be81f


          Fix indentation

ff64dbc

Contributor Author

LarsDenBakker commented Jun 7, 2018

@aomarks Made the changes. Also, I've run the tests on chrome, firefox, safari, edge and IE11.

Lars den Bakker and others added 2 commits

June 7, 2018 09:50


          Merge branch 'upstream-master' into feature/resolve-html-import-url

9a7aea5


          Merge branch 'master' into feature/resolve-html-import-url

1c955f9

aomarks approved these changes

View reviewed changes

aomarks merged commit d4ab1d5 into Polymer:master

snyk-bot mentioned this pull request

[Snyk] Fix for 1 vulnerabilities gcloud-lerralice/tools#25

Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels