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

module-types use assert type to detect module type #2434

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 13 additions & 1 deletion docs/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -345,4 +345,16 @@ If you're only using named AMD modules as part of a script loaded by System.impo

The reason for this is that SystemJS generally identifies modules by their URLs - one URL per module. Import Maps are the primary method to alias a bare specifier to a URL, but it's also possible to identify modules by a name without specifying a URL for each module, by creating named System.register or named AMD modules.

When the SystemJS amd.js extra's `define` function is given a named AMD module, the module is identified by the URL of the currently executing script, if the script was loaded via `System.import()`. However, if the script was not loaded by `System.import()` or if there are multiple defines in the same script, then the module(s) will not have a URL associated with them and therefore will not be accessible by any identifier. To solve this problem, the named-register.js tracks all named modules (including named AMD modules and named System.register modules) by name, rather than by URL. This makes it possible to have one script that registers multiple named modules.
When the SystemJS amd.js extra's `define` function is given a named AMD module, the module is identified by the URL of the currently executing script, if the script was loaded via `System.import()`. However, if the script was not loaded by `System.import()` or if there are multiple defines in the same script, then the module(s) will not have a URL associated with them and therefore will not be accessible by any identifier. To solve this problem, the named-register.js tracks all named modules (including named AMD modules and named System.register modules) by name, rather than by URL. This makes it possible to have one script that registers multiple named modules.

## W7

### Import assertion failed

When using import assertion like

```js
import { foo } from 'foo.js' assert { type: 'json' };
```
Comment on lines +354 to +358
Copy link

Choose a reason for hiding this comment

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

Suggested change
When using import assertion like
```js
import { foo } from 'foo.js' assert { type: 'json' };
```
When using import attributes like
```js
import { foo } from 'foo.js' with { type: 'json' };
```


User should ensure the server response header contains the correct MIME type.
12 changes: 11 additions & 1 deletion docs/module-types.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,22 @@ SystemJS supports loading modules that are in the following formats:
| [AMD](https://github.com/amdjs/amdjs-api/wiki/AMD) | [AMD extra](/dist/extras/amd.js) | [AMD extra](/dist/extras/amd.js) | * |
| [UMD](https://github.com/umdjs/umd) | [AMD extra](/dist/extras/amd.js) | [AMD extra](/dist/extras/amd.js) | * |

### File Extension Limitations
### Module type detection

When loading JSON modules, CSS modules and Web Assembly modules, the browser specifications require interpreting these modules based on checking their MIME type. Since SystemJS has to choose upfront whether to append a script element (for JS modules) or make a fetch request (for a JSON/CSS/Wasm module), it needs to know the module type upfront at resolution time.

Instead of reading the MIME type, the file extension is thus used specifically for the JSON, CSS and Web Assembly module cases.

If module type is specified by import, the mime is acceptable as the module type, will use the specified module type.

e.g.

```js
System.import('https://example.com/my-module.txt', {assert: {type: 'css'}});
Copy link

Choose a reason for hiding this comment

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

Suggested change
System.import('https://example.com/my-module.txt', {assert: {type: 'css'}});
System.import('https://example.com/my-module.txt', {with: {type: 'css'}});

```

This will force resolve the module as a CSS module, even if the file extension is `.txt` and the MIME type is `text/plain`.

## JSON Modules

[JSON modules](https://github.com/whatwg/html/pull/4407) support importing a JSON file as the default export.
Expand Down
93 changes: 61 additions & 32 deletions src/extras/module-types.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { resolveUrl } from '../common.js';
import {errMsg} from '../err-msg.js';

/*
* Loads JSON, CSS, Wasm module types based on file extension
Expand All @@ -9,35 +10,32 @@ import { resolveUrl } from '../common.js';

var moduleTypesRegEx = /^[^#?]+\.(css|html|json|wasm)([?#].*)?$/;
var _shouldFetch = systemJSPrototype.shouldFetch.bind(systemJSPrototype)
systemJSPrototype.shouldFetch = function (url) {
return _shouldFetch(url) || moduleTypesRegEx.test(url);
systemJSPrototype.shouldFetch = function (url, parent, meta) {
var assertType = ((meta || {}).assert || {}).type;
return _shouldFetch(url, parent, meta) || moduleTypesRegEx.test(url) || !!loaders[assertType];
Comment on lines +14 to +15
Copy link

Choose a reason for hiding this comment

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

Suggested change
var assertType = ((meta || {}).assert || {}).type;
return _shouldFetch(url, parent, meta) || moduleTypesRegEx.test(url) || !!loaders[assertType];
var moduleType = ((meta || {}).with || {}).type;
return _shouldFetch(url, parent, meta) || moduleTypesRegEx.test(url) || !!loaders[moduleType];

};

var jsonContentType = /^application\/json(;|$)/;
var cssContentType = /^text\/css(;|$)/;
var wasmContentType = /^application\/wasm(;|$)/;

var fetch = systemJSPrototype.fetch;
systemJSPrototype.fetch = function (url, options) {
return fetch(url, options)
.then(function (res) {
if (options.passThrough)
return res;

if (!res.ok)
return res;
var contentType = res.headers.get('content-type');
if (jsonContentType.test(contentType))
return res.json()
var fallback = '_fallback';
var contentTypes = [
[/^application\/json(;|$)/, 'json'],
[/^text\/css(;|$)/, 'css'],
[/^application\/wasm(;|$)/, 'webassembly'],
[/^text\/plain(;|$)/, fallback],
[/^application\/octet-stream(;|$)/, fallback],
]
var loaders = {
'json': function (res) {
return res.json()
.then(function (json) {
return new Response(new Blob([
'System.register([],function(e){return{execute:function(){e("default",' + JSON.stringify(json) + ')}}})'
], {
type: 'application/javascript'
}));
});
if (cssContentType.test(contentType))
return res.text()
})
},
'css': function (res, url) {
return res.text()
.then(function (source) {
source = source.replace(/url\(\s*(?:(["'])((?:\\.|[^\n\\"'])+)\1|((?:\\.|[^\s,"'()\\])+))\s*\)/g, function (match, quotes, relUrl1, relUrl2) {
return 'url(' + quotes + resolveUrl(relUrl1 || relUrl2, url) + quotes + ')';
Expand All @@ -48,11 +46,11 @@ import { resolveUrl } from '../common.js';
type: 'application/javascript'
}));
});
if (wasmContentType.test(contentType))
return (WebAssembly.compileStreaming ? WebAssembly.compileStreaming(res) : res.arrayBuffer().then(WebAssembly.compile))
},
'webassembly': function (res, url) {
return (WebAssembly.compileStreaming ? WebAssembly.compileStreaming(res) : res.arrayBuffer().then(WebAssembly.compile))
.then(function (module) {
if (!global.System.wasmModules)
global.System.wasmModules = Object.create(null);
if (!global.System.wasmModules) global.System.wasmModules = Object.create(null);
global.System.wasmModules[url] = module;
// we can only set imports if supported (eg early Safari doesnt support)
var deps = [];
Expand All @@ -65,15 +63,46 @@ import { resolveUrl } from '../common.js';
setterSources.push('function(m){i[' + key + ']=m}');
}
});
return new Response(new Blob([
'System.register([' + deps.join(',') + '],function(e){var i={};return{setters:[' + setterSources.join(',') +
'],execute:function(){return WebAssembly.instantiate(System.wasmModules[' + JSON.stringify(url) +
'],i).then(function(m){e(m.exports)})}}})'
], {
return new Response(new Blob(['System.register([' + deps.join(',') + '],function(e){var i={};return{setters:[' + setterSources.join(',') + '],execute:function(){return WebAssembly.instantiate(System.wasmModules[' + JSON.stringify(url) + '],i).then(function(m){e(m.exports)})}}})'], {
type: 'application/javascript'
}));
});
return res;
});
}
}

var fetch = systemJSPrototype.fetch;
systemJSPrototype.fetch = function (url, options) {
Copy link
Member

Choose a reason for hiding this comment

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

I missed that we were passing the assertion directly as options here. Does that mean that you can set custom fetch options this way as well via the new System.register attributes?

return fetch(url, options)
.then(function (res) {
if (options.passThrough) return res;

if (!res.ok) return res;

var assertType = ((options.meta || {}).assert || {}).type;
var mime = res.headers.get('content-type');
var contentType
for (var i = 0; i < contentTypes.length; i++) {
var rule = contentTypes[i];
if (rule[0].test(mime)) {
contentType = rule[1]
break
}
}
Comment on lines +82 to +90
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there's a shorter way to do this?


// ensure type match
if (assertType && contentType){
if(contentType === fallback){
contentType = assertType
}
if (assertType !== contentType){
console.warn(process.env.SYSTEM_PRODUCTION ? errMsg('W7') : errMsg('W7', `Module ${url} has an unexpected content type of "${mime}": expected module type ${assertType} got ${contentType}.`));
contentType = undefined
}
}
Comment on lines +93 to +101
Copy link
Member

Choose a reason for hiding this comment

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

This seems fine to me, just watch the spacing of the syntax. Also shorter code is always better for this lib!


var loader = loaders[contentType];
if (loader) return loader(res, url, options);
return res;
});
};
})(typeof self !== 'undefined' ? self : global);
16 changes: 16 additions & 0 deletions test/browser/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,22 @@ suite('SystemJS Standard Tests', function() {
});
});

test('should load a.txt as css module by assert', function () {
return System.import('fixturesbase/css-modules/a.txt', {assert: {type: 'css'}}).then(function (m) {
Comment on lines +230 to +231
Copy link

Choose a reason for hiding this comment

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

Suggested change
test('should load a.txt as css module by assert', function () {
return System.import('fixturesbase/css-modules/a.txt', {assert: {type: 'css'}}).then(function (m) {
test('should load a.txt as css module by type attribute', function () {
return System.import('fixturesbase/css-modules/a.txt', {with: {type: 'css'}}).then(function (m) {

assert.ok(m);
assert.ok(isCSSStyleSheet(m.default));
document.adoptedStyleSheets = document.adoptedStyleSheets.concat(m.default);
});
});

test('should load a as css module by assert', function () {
return System.import('fixturesbase/css-modules/a', {assert: {type: 'css'}}).then(function (m) {
Comment on lines +238 to +239
Copy link

Choose a reason for hiding this comment

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

Suggested change
test('should load a as css module by assert', function () {
return System.import('fixturesbase/css-modules/a', {assert: {type: 'css'}}).then(function (m) {
test('should load a as css module by type attribute', function () {
return System.import('fixturesbase/css-modules/a', {with: {type: 'css'}}).then(function (m) {

assert.ok(m);
assert.ok(isCSSStyleSheet(m.default));
document.adoptedStyleSheets = document.adoptedStyleSheets.concat(m.default);
});
});

test('should support application/javascript css module override', function () {
return System.import('fixturesbase/css-modules/javascript.css').then(function (m) {
assert.ok(m);
Expand Down
6 changes: 6 additions & 0 deletions test/fixtures/css-modules/a
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
.hello {
background-color: peru;
}
body {
background-color: lightblue;
}
6 changes: 6 additions & 0 deletions test/fixtures/css-modules/a.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
.hello {
background-color: peru;
}
body {
background-color: lightblue;
}