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

wenerme · 2022-10-05T02:29:54Z

No description provided.

wenerme · 2022-10-05T02:30:30Z

github-actions · 2022-10-05T02:30:50Z

File size impact

Merging module-types into main will impact 7 files in 3 groups.

browser (2/2)

File	raw	gzip	brotli	Event
dist/s.min.js	-105 (7,776)	-54 (3,069)	-56 (2,784)	modified
dist/system.min.js	-109 (12,110)	-53 (4,668)	-59 (4,198)	modified
Total size impact	-214 (19,886)	-107 (7,737)	-115 (6,982)

node (1/1)

File	raw	gzip	brotli	Event
dist/system-node.cjs	-1,674 (517,525)	-273 (125,094)	-251 (84,042)	modified
Total size impact	-1,674 (517,525)	-273 (125,094)	-251 (84,042)

extras (4/8)

File	raw	gzip	brotli	Event
dist/extras/global.min.js	-4 (861)	-1 (508)	-2 (428)	modified
dist/extras/module-types.min.js	+581 (3,434)	+295 (1,526)	+275 (1,356)	modified
dist/extras/named-register.min.js	-10 (993)	-6 (464)	-2 (399)	modified
dist/extras/transform.min.js	-4 (671)	-2 (408)	-33 (335)	modified
Total size impact	+563 (5,959)	+286 (2,906)	+238 (2,518)

_{Generated by file size impact during size-impact#3198880154}

guybedford · 2022-10-05T18:34:04Z

Unfortunately, this is not how assertions are supposed to work - the content-type header is supposed to be the authoritative source for the module type information, not the assertion. https://github.com/tc39/proposal-import-assertions#proposed-semantics-and-interoperability

wenerme · 2022-10-06T07:22:06Z

Now module-types will do the assert, but can fallback to given type if mime is acceptable based on https://github.com/tc39/proposal-import-assertions/blob/master/content-type-vs-file-extension.md , do you thinks is this ok ?

this pr need #2435

guybedford · 2022-10-06T15:36:13Z

Unfortunately the assertion can only be a validation and cannot affect the way we do fetching.

wenerme · 2022-10-09T01:16:10Z

@guybedford changed throw to a warning log, is this ok ?

guybedford · 2022-10-10T14:52:52Z

src/extras/module-types.js

+  }
+
+  var fetch = systemJSPrototype.fetch;
+  systemJSPrototype.fetch = function (url, options) {


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?

guybedford · 2022-10-10T14:53:22Z

src/extras/module-types.js

+        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
+          }
+        }


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

guybedford · 2022-10-10T14:53:50Z

src/extras/module-types.js

+        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
+          }
+        }


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

guybedford

Yes, we can add this, let's just clarify how we are exposing the fetch options to the assertions system? Does that mean System.register([], () => {...}, [{ headers: { ... } }]) can eg customize fetch options?

wenerme · 2022-10-12T04:00:50Z

@guybedford Passing header is possible, but should or shouldn't is a question, for me, the Meta just like a black box with minimal know info, every hook can use this to pass addition options, if Meta is passing all the way along, the Meta became the context, this make more sense.

wenerme · 2022-10-12T04:06:10Z

BTW, for hook level customization, I prefer the createHook pattern like

https://github.com/wenerme/wode/blob/19c21023dd6ea68cc4fd9fd73423927c5faa7237/packages/system/src/loaders/hookSystem.ts#L28-L29

meta is better to pass one-shot options.

But systemjs is packed as iife, so the createHook pattern is impossible, for global wise customization/options, user can override the hook and always patch the meta, but I don't know is this a good practice.

wenerme · 2022-10-12T04:13:32Z

I think meta should not used for extend assertions actions (e.g. extra loader, add more content type), just passing extra fetch option is acceptable, like

override the request init

System.register([], () => {...}, [{ request:{headers:{}} }])

or always use fetch as key

System.register([], () => {...}, [{ fetch:{headers:{}} }])

or override fetch

System.register([], () => {...}, [{ fetch:(u,o)=>fetch(u,{...o,headers:{}}) }])

Just adding headers will became confusing when there is a lot options for RequestInit, e.g. allowed passing cookie credential.

guybedford · 2022-10-12T14:55:38Z

Passing header is possible, but should or shouldn't is a question, for me

This is what concerns me - perhaps we should restrict the functionality for now rather?

ExE-Boss

The spec was changed to re‑use the with keyword and renamed to “import attributes”:

Allow assertions to affect module loading, change keyword to with tc39/proposal-import-attributes#131
Rename from "assertions" to "attributes" tc39/proposal-import-attributes#134

ExE-Boss · 2023-05-01T06:57:50Z

docs/errors.md

+
+```js
+import { foo } from 'foo.js' assert { type: 'json' };
+```


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

```

ExE-Boss · 2023-05-01T06:58:22Z

docs/module-types.md

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


Suggested change

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

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

ExE-Boss · 2023-05-01T06:59:29Z

src/extras/module-types.js

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


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];

ExE-Boss · 2023-05-01T07:01:40Z

test/browser/core.js

+  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) {


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

ExE-Boss · 2023-05-01T07:01:52Z

test/browser/core.js

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


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

wenerme force-pushed the module-types branch from d94edeb to 3eda65d Compare October 5, 2022 02:36

wenerme force-pushed the module-types branch from 3eda65d to 28005cd Compare October 6, 2022 07:19

wenerme force-pushed the module-types branch from 28005cd to a3dc9f6 Compare October 6, 2022 07:53

wenerme mentioned this pull request Oct 6, 2022

add import assertions to system-register.md #2436

Merged

wenerme force-pushed the module-types branch 2 times, most recently from aa88e31 to 4e6cdd1 Compare October 6, 2022 16:33

module-types check assert type, allowed fallback to specified type.

cad5ab3

wenerme force-pushed the module-types branch from 4e6cdd1 to cad5ab3 Compare October 6, 2022 16:34

guybedford reviewed Oct 10, 2022

View reviewed changes

guybedford force-pushed the main branch from 49534c7 to 6d95f11 Compare February 25, 2023 16:07

ExE-Boss reviewed May 1, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wenerme commented Oct 5, 2022

wenerme commented Oct 5, 2022

github-actions bot commented Oct 5, 2022 •

edited

guybedford commented Oct 5, 2022

wenerme commented Oct 6, 2022

guybedford commented Oct 6, 2022

wenerme commented Oct 9, 2022

guybedford Oct 10, 2022

guybedford Oct 10, 2022

guybedford Oct 10, 2022

guybedford left a comment

wenerme commented Oct 12, 2022

wenerme commented Oct 12, 2022 •

edited

wenerme commented Oct 12, 2022

guybedford commented Oct 12, 2022

ExE-Boss left a comment

ExE-Boss May 1, 2023

ExE-Boss May 1, 2023

ExE-Boss May 1, 2023

ExE-Boss May 1, 2023

ExE-Boss May 1, 2023

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

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

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

		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 as css module by assert', function () {
		return System.import('fixturesbase/css-modules/a', {assert: {type: 'css'}}).then(function (m) {

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

Are you sure you want to change the base?

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

Conversation

wenerme commented Oct 5, 2022

wenerme commented Oct 5, 2022

github-actions bot commented Oct 5, 2022 • edited

File size impact

guybedford commented Oct 5, 2022

wenerme commented Oct 6, 2022

guybedford commented Oct 6, 2022

wenerme commented Oct 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford left a comment

Choose a reason for hiding this comment

wenerme commented Oct 12, 2022

wenerme commented Oct 12, 2022 • edited

wenerme commented Oct 12, 2022

guybedford commented Oct 12, 2022

ExE-Boss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 5, 2022 •

edited

wenerme commented Oct 12, 2022 •

edited