module: add import map support #50590

wesleytodd · 2023-11-07T06:52:45Z

This PR adds experimental support for import maps behind the --experimental-import-map=<path> flag. This is a draft as I believe there are still quite a few things to discuss and add:

Helpful links:

Initial issue from the module/loaders group: Support for Import Maps #49443
Import map spec: https://html.spec.whatwg.org/multipage/webappapis.html#import-maps
Import map wicg repo: https://github.com/WICG/import-maps
Recent plan issue: Import Maps Implementation Plan loaders#168

nodejs-github-bot · 2023-11-07T06:52:50Z

Review requested:

@nodejs/loaders

lib/internal/modules/esm/import_map.js

lib/internal/modules/run_main.js

lib/internal/modules/esm/import_map.js

lib/internal/modules/run_main.js

src/node_options.cc

test/fixtures/es-module-loaders/importmaps/array-imports.json

test/es-module/test-importmap.mjs

lib/internal/modules/run_main.js

aduh95 · 2023-11-07T09:36:06Z

lib/internal/modules/esm/loader.js

@@ -391,6 +396,7 @@ class ModuleLoader {
      conditions: this.#defaultConditions,
      importAttributes,
      parentURL,
+      importMap: this.importMap,


I don't think this is transferable, meaning this will throw as soon as any custom loader is registered. Can you add a test with a loader?

Yeah, this currently doesn't work. Frankly I am not sure how we do want it to work. Do we want to pass just the "raw" import map object and add handling inside the worker to re-hydrate the ImportMap class with it? I am struggling to decide if there is a good use case to expose this to other loaders. Would it instead make sense to remove it from context before passing context to the worker?

Yup. Policies have to xfer the source. Take note though that if Workers are allowed to override the value in your design (since they can manipulate argv)

Can you represent the import map value as a plain object that’s transferable? Like I would assume you should be able to implement toJSON and fromJSON methods to your class, and JSON is transferable. Not that I’m suggesting going as far as JSON when an object will do, but if toJSON/fromJSON is possible then some kind of transferable data structure is therefore also possible.

Can you represent the import map value as a plain object that’s transferable?

Yep, that is what I meant to say by "Do we want to pass just the raw import map object and add handling inside the worker to re-hydrate the ImportMap class". I just am not sure this makes sense to do. Not only would it be slow but I also am not sure I can think of a use case for accessing/changing the import map in a loader.

Ok, so the initial non-transferable thing was actually the parent URL instance. I resolved that. Now we have a new question of if, when using the customized loader, the import map resolve should happen inside the worker or on the main thread.

I just pushed an implementation where it happens on the main thread, which means that the processing is on the main thread both when using a custom loader and not. What are the benefits of doing the import map resolution in the worker thread.

cc @JakobJingleheimer maybe we continue this conversation here instead of slack?

Ok, in slack I think @GeoffreyBooth made a good case for this needing to be on context. I think that means I need to make it load the import map inside the worker when using custom loader hooks.

I still think I can avoid reading it twice by checking if there are customization and only loading it on the main thread when there are not.

aduh95 · 2023-11-07T15:24:28Z

Could you also fix the commit message to e.g. module: add import map support please? docs is not a valid subsystem, and src and lib are way too broad for that change.

wesleytodd · 2023-11-07T16:46:08Z

Could you also fix the commit message to e.g. module: add import map support please? docs is not a valid subsystem, and src and lib are way too broad for that change.

I actually did that at first but then the PR docs and other examples I saw seemed to suggest this was the right way. And the validation already caught docs and I changed it to doc just did not edit the PR title (doing that now). Maybe we need better docs on this?

GeoffreyBooth

Looks quite good! We also need to add documentation for this. I would add a section to https://github.com/nodejs/node/blob/main/doc/api/module.md, maybe just above “Customization Hooks.” We also need to document the flag in https://github.com/nodejs/node/blob/main/doc/api/cli.md.

lib/internal/modules/esm/import_map.js

GeoffreyBooth · 2023-11-07T17:11:39Z

lib/internal/modules/esm/import_map.js

+      throw new ERR_INVALID_IMPORT_MAP('module specifier values must be non-empty strings');
+    }
+    if (specifier.endsWith('/') && !mapping.endsWith('/')) {
+      throw new ERR_INVALID_IMPORT_MAP('module specifier values for keys ending with / must also end with /');


Suggested change

throw new ERR_INVALID_IMPORT_MAP('module specifier values for keys ending with / must also end with /');

throw new ERR_INVALID_IMPORT_MAP('module specifier keys ending with "/" must have values that end with "/"');

We have generic helpers like validateString, if there isn’t a validatePlainObject it might be worth creating one.

Not sure why github did not give me a comment box to directly reply to that comment, but here it is. I think that would be good idea and I can add them in this PR if they did not exist. Where would they live if I wanted to go looking for methods like this?`

lib/internal/modules/esm/import_map.js

lib/internal/modules/esm/resolve.js

src/node_options.h

test/es-module/test-importmap.mjs

GeoffreyBooth · 2023-11-07T17:19:01Z

test/es-module/test-importmap.mjs

+import import_map from 'internal/modules/esm/import_map';
+const { ImportMap } = import_map;
+import binding from 'internal/test/binding';
+const { primordials: { SafeMap, JSONStringify } } = binding;


In general, you don’t need to use primordials in tests.

The linter told me otherwise IIRC. Happy to do whatever folks recommend, just wanted to call out that I did this because the linter told me to lol.

test/es-module/test-importmap.mjs

wesleytodd · 2023-11-11T21:44:40Z

So I looked to see if there was a good way to use the web platform tests, but all of these ones are html tests. For now I just went through and made sure that I covered the gaps I had seen in my original tests instead of using wpt directly (hence marking that complete in the OP). This led to cleaning up some bare specifier things, as well as a bunch of data uri handling stuff. It made it clear I needed to clean up the fixtures, so I did that as well. I think now the last tests I am missing are for two things

bare/sub-path in imports
http imports with other url parts (host, query params, etc)

lib/internal/modules/esm/import_map.js

anonrig · 2023-11-12T00:33:59Z

lib/internal/modules/helpers.js

+ */
+function shouldBeTreatedAsRelativeOrAbsolutePath(specifier) {
+  if (specifier === '') { return false; }
+  if (specifier[0] === '/') { return true; }


Shouldn't this be path.sep, since Windows have a different path separator?

I moved this so I could re-use it in import_map.js, it is just as it was before I moved it. I am thinking maybe this is because imports/require statements are all using nix style, but honestly I am not certain without digging around a bit.

test/es-module/test-import-map.mjs

lib/internal/modules/esm/import_map.js

anonrig · 2023-11-12T00:40:46Z

lib/internal/modules/helpers.js

+ * Determines whether a specifier is a relative path.
+ * @param {string} specifier - The specifier to check.
+ */
+function isRelativeSpecifier(specifier) {


Moving to a different file should be implemented in a different commit for future observability.

Happy to do that, do folks typically do that as different PRs or multiple commits in the one PR?

lib/internal/modules/run_main.js

guybedford · 2023-11-19T00:50:34Z

Thank you @wesleytodd for pushing this again, I was hoping to do a review here when it is ready, but to comment on a couple of the recent points:

For WPT there are data-driven tests in https://github.com/web-platform-tests/wpt/tree/master/import-maps/data-driven/resources which are not at all HTML based.

Can we please unmark the WPT checkbox until all those data-driven tests pass?

If we put import maps on context will they be regarded as mutable? Mutability of import maps when exposing them to userland is an important question to answer there, since there are fundamental resolver concerns around mutability for import maps, which are hard to answer if we need to address that question now.

wesleytodd · 2023-11-20T20:00:17Z

I was hoping to do a review here when it is ready

Thanks, I was going to ping you directly when I had what I considered a "ready" state, but thanks for chiming in and if there is anything I can do to ease the review let me know.

Can we please unmark the WPT checkbox until all those data-driven tests pass?

Done.

or WPT there are data-driven tests in

I will look into these the next time I have to work on this. Should be this week sometime.

will they be regarded as mutable?

Yeah, this is my concern for the loaders accessing it as well. I am not sure I understand the use case we for this, and IMO it is easier to expose it later than to try and backtrack around folks doing things we didn't expect.

which are hard to answer if we need to address that question now.

I think we do need to address them. I would rather not expose them until we had a clear understanding of why so we can write good tests of the use case. Is there a good way to avoid exposing them while also not putting it on context?

wesleytodd · 2023-11-21T17:33:39Z

Wanted to make sure I captured this here, from a slack convo with @guybedford:

We want to make the imports and scopes getters return plain objects which are converted back from the safe maps into their original format. This will simplify the tests, better meet end user expectations, and also avoid any mutation issues by exposing this on context.

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Nov 7, 2023

wesleytodd force-pushed the import-maps branch from cf66b50 to 196df55 Compare November 7, 2023 06:54

ljharb suggested changes Nov 7, 2023

View reviewed changes

aduh95 reviewed Nov 7, 2023

View reviewed changes

wesleytodd changed the title ~~lib,src,docs: add import map support~~ lib,src,doc: add import map support Nov 7, 2023

wesleytodd changed the title ~~lib,src,doc: add import map support~~ module: add import map support Nov 7, 2023

wesleytodd force-pushed the import-maps branch from 196df55 to 5663357 Compare November 7, 2023 16:47

GeoffreyBooth reviewed Nov 7, 2023

View reviewed changes

JakobJingleheimer added module Issues and PRs related to the module subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders labels Nov 7, 2023

JakobJingleheimer added this to In progress in Loaders via automation Nov 7, 2023

JakobJingleheimer added this to the ESM Loaders: Primetime milestone Nov 7, 2023

wesleytodd force-pushed the import-maps branch 9 times, most recently from b60b205 to 5b6df57 Compare November 11, 2023 21:40

anonrig reviewed Nov 12, 2023

View reviewed changes

wesleytodd force-pushed the import-maps branch from 5b6df57 to 0afe83f Compare November 17, 2023 17:14

wesleytodd force-pushed the import-maps branch 4 times, most recently from b0a4cbb to a59b5e5 Compare November 17, 2023 17:42

module: add import map support

a32030d

wesleytodd force-pushed the import-maps branch from a59b5e5 to a32030d Compare November 17, 2023 17:54

This comment was marked as spam.

Sign in to view

wesleytodd mentioned this pull request Apr 8, 2024

Support for Import Maps #49443

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: add import map support #50590

module: add import map support #50590

wesleytodd commented Nov 7, 2023 •

edited

nodejs-github-bot commented Nov 7, 2023

aduh95 Nov 7, 2023

wesleytodd Nov 11, 2023

bmeck Nov 12, 2023

GeoffreyBooth Nov 12, 2023

wesleytodd Nov 13, 2023

wesleytodd Nov 17, 2023 •

edited

wesleytodd Nov 17, 2023

aduh95 commented Nov 7, 2023

wesleytodd commented Nov 7, 2023

GeoffreyBooth left a comment

GeoffreyBooth Nov 7, 2023

wesleytodd Nov 7, 2023

GeoffreyBooth Nov 7, 2023

wesleytodd Nov 7, 2023

wesleytodd commented Nov 11, 2023 •

edited

anonrig Nov 12, 2023

wesleytodd Nov 12, 2023

anonrig Nov 12, 2023

wesleytodd Nov 12, 2023

guybedford commented Nov 19, 2023

wesleytodd commented Nov 20, 2023

wesleytodd commented Nov 21, 2023

This comment was marked as spam.

	throw new ERR_INVALID_IMPORT_MAP('module specifier values for keys ending with / must also end with /');
	throw new ERR_INVALID_IMPORT_MAP('module specifier keys ending with "/" must have values that end with "/"');

module: add import map support #50590

Are you sure you want to change the base?

module: add import map support #50590

Conversation

wesleytodd commented Nov 7, 2023 • edited

nodejs-github-bot commented Nov 7, 2023

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

wesleytodd Nov 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduh95 commented Nov 7, 2023

wesleytodd commented Nov 7, 2023

GeoffreyBooth 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

wesleytodd commented Nov 11, 2023 • edited

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

guybedford commented Nov 19, 2023

wesleytodd commented Nov 20, 2023

wesleytodd commented Nov 21, 2023

This comment was marked as spam.

wesleytodd commented Nov 7, 2023 •

edited

wesleytodd Nov 17, 2023 •

edited

wesleytodd commented Nov 11, 2023 •

edited