Open wesleytodd opened 1 year ago
Hey, thank you for spearheading this! I had a few thoughts:
When we implemented --env-file
, the initial plan was to have a “well known” local file like .env
that would be loaded automatically. We had to backtrack on that because that would’ve made it a semver-major change. We might still automatically load .env
at some point in the future, though, like maybe in Node 22. But anyway I would suggest you start similarly, like have your flag take a value like --import-map=import-map.json
(I’m not sure we need experimental
in the name). That provides more flexibility anyway, in case the user wants to tell Node to use an import map that isn’t the default one (like if there are different import maps for dev versus prod builds).
Whenever module customization hooks are registered, a new thread is spawned to run them. This is unnecessary overhead for what import maps is doing; import maps is basically an alternative ESM resolution algorithm, that can be run sync just like the existing resolution algorithm is. Take a look at https://github.com/nodejs/node/blob/05a7810a1d9e088f03be1f99e6d3eba3d06e2571/lib/internal/modules/esm/resolve.js#L1029; you’ll see at the top of defaultResolve
there’s already an alternative resolution algorithm, for policy manifest redirects. I would put the import maps handling in this function, so that it’s handled within the defaultResolve
that’s already at the end of the resolve
hook chain. You just need to ensure that your implementation is sync so that defaultResolve
can remain a sync function.
You should find tests from somewhere and include them in our codebase. Like I don’t know if there are Web Platform Tests for import maps, but those would be great to pull in; or maybe adapt Deno’s. Ideally we can reuse some comprehensive test suite to ensure that our implementation fulfills the standard.
have your flag take a value like --import-map=import-map.json
Yes, exactly 🙂
Whenever module customization hooks are registered, a new thread is spawned to run them.
It would not here. It would be part of the "default".
You should find tests from somewhere and include them in our codebase.
Wes is currently working on this 🙂
Hey folks, I think I have what we could open as the original PR. I was hoping to have it open already, so figured I would just gauge folks opinions on leaving opening it as is. There are some still unresolved things and I wanted to check if we think we need them for the initial PR (while fully recognizing that we need them before we merge).
Here is the current status: https://github.com/wesleytodd/node/pull/1/files
I would love any comments over there which I can address before opening up the node PR.
I would love any comments over there which I can address before opening up the node PR.
Done. I also don’t think you need to fear opening a PR. You can open it as draft and include a TODO list in the top post explaining what work remains to be done before it’s ready for review.
Cool, I just wanted to make sure I avoided any major bike-shedding or mistakes since it was my first and it is already reaching the limits of my available time commitment. Thanks everyone for the comments, I will address them and then open the PR as a draft (hopefully this week).
Hey folks! @JakobJingleheimer and I sync'd up this week and discussed a plan for an Import Maps PR. The first step was to start an issue here with a more concrete technical plan, so here it is. Just to be clear: this is a plan proposal, all details are up for discussion I just wanted to get the starting point for those discussions.
Goal
The ultimate goal is import maps enabled by default if a well known file is present (ex
./importmap.json
). If no file is found at that location, the loader is not added to the chain. The file location can be changed with a cli flag (ex--import-map
), which will also turn on the loader and throw if the import map is unavailable or malformed.The loader should apply its transformations before custom loaders, and it should work well with other existing features (ex exports, policies, etc). I am intentionally leaving some details light on what it means to "work well with other existing features", my hope is we can identify those areas and what requirements we want to put on the implementation in this discussion.
The ideal end user experience here is that their package manager of choice generates an import map for them and it is found/used without additional user intervention. Secondarily, tool authors or users could hand write (or generate) import maps which provide more bespoke features.
Getting There
To get to this end goal, we plan to start with an experimental loader and flag. This initial approach does not need to get 100% coverage, we will take an iterative approach with a goal of 80% of use cases covered in the initial PR. The initial PR will be behind this experimental flag (ex
--experimental-import-map
). Before that, we need to decide to either write a new loader or pull in an existing implementation from userland. We have a few examples we can reference:@joeldenning
's: https://github.com/node-loader/node-loader-import-maps/blob/master/lib/node-import-map-loader.jsOpen Questions
importmap.json
,.importmap.json
,node_modules/importmap.json
, etc)Next steps