-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Import maps don't work with relative url as a module specifier #2038
Comments
From the import maps spec:
SystemJS has a unit test that verifies that relative-URL-like address are resolved correctly: Line 98 in e7af7c9
Could you clarify what you mean by the following?
What exactly isn't supported? A minimal repo or codesandbox that doesn't require a build step would likely help us diagnose this the best. A difference from the experimental Chrome implementation is concerning (although we'll have to look into which of the two implementations is the one that's wrong!) |
Here's a test case that fails (adopting an example from the other tests in the file you linked): it('Can map relative URLs', function () {
assert.equal(doResolveImportMap('./x.js', 'https://site.com/', {
imports: {
'./x.js': './x-1234.js',
},
scopes: {},
}), 'https://site.com/x-1234.js');
}); Here's the result I see when running the above: 1) Import Maps
Can map relative URLs:
AssertionError [ERR_ASSERTION]: 'https://site.com/x.js' == 'https://site.com/x-1234.js'
+ expected - actual
-https://site.com/x.js
+https://site.com/x-1234.js
at Context.<anonymous> (test/import-map.js:55:12)
at process.topLevelDomainCallback (domain.js:120:23) Does that help clarify the issue? |
Ah yes thanks for the detailed info. I didn't read your original description carefully to realize you were talking about the module specifiers (keys in the object), not the urls (values). From different part of import maps spec:
So yeah, this is a bug in SystemJS. Could you put together a fix for this? If not, myself or Guy Bedford can take a look at it later. |
Hey @philipwalton, thanks for reporting this. The code you were stepping through assumes that is applying to the resolved import map, but we weren't handling this resolution in the import map normalization itself. Fixed in #2039. |
Thanks for the quick fix! I'm working on a demo showing some examples of using import maps, and I'd love to show SystemJS in that demo if this fix gets released before that. |
@philipwalton sure we usually release fairly regularly so should be able to do the release in the next day or two. When's the demo? |
Next day or two should be fine. I plan to publish an article about this early/mid next week, and it'll include demos which can be updated after the fact. At the moment the demos are linking to a custom version of systemjs built from master (which works great), so it's not critical if you can't release in time. But obviously it would look better if the demos don't include a custom version. |
Nice to hear, if you'd like reviews feel free to ping us too. Released in 6.1.3. |
I'm working on a demo that showing usage of Import Maps with fallback to SystemJS, and I'm realizing that mapping from relative URLs (to other URLs) doesn't seem to be supported.
For example, here's my import map (which works as a native
importmap
, but not with SystemJS 6.1.0):In stepping through the code to see what's not working, it looks like this function doesn't account for the possibility of the object keys being relative URLs (as it only does a substring match):
systemjs/dist/system.js
Lines 154 to 163 in e7af7c9
The text was updated successfully, but these errors were encountered: