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

Import Maps Implementation Plan #168

Open
wesleytodd opened this issue Oct 20, 2023 · 5 comments
Open

Import Maps Implementation Plan #168

wesleytodd opened this issue Oct 20, 2023 · 5 comments

Comments

@wesleytodd
Copy link
Member

wesleytodd commented Oct 20, 2023

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:

  1. @joeldenning's: https://github.com/node-loader/node-loader-import-maps/blob/master/lib/node-import-map-loader.js
  2. my original example: https://gist.github.com/wesleytodd/4399b2351c59438db19a8ffb1f3fcdca
  3. ...add more here...

Open Questions

  1. An alternative to not loading the loader would be to always have it but use an empty import map as a fallback. Good idea? Bad Idea?
  • Highly likley a no for performance reasons. For reference, the spec includes that all Document's have an empty one but we believe this is not important for our use case.
  1. what should the flag be (both experimental and final)?
  2. where should the well known file be by default? (importmap.json, .importmap.json, node_modules/importmap.json, etc)
  3. Scope resolution: for large import maps it seems like we want to implement a radix tree lookup? Should we even start with a naive loop approach or just do the radix tree right away?
  4. Should import maps work with commonjs?

Next steps

  1. open the pr?
  2. profit?
@GeoffreyBooth
Copy link
Member

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.

@JakobJingleheimer
Copy link
Contributor

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 🙂

@wesleytodd
Copy link
Member Author

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

  1. http import support: we discussed in slack and for sure want the two experiments to work together
  2. data urls: again, discussed and for sure need to support them, just need to work through details

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.

@GeoffreyBooth
Copy link
Member

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.

@wesleytodd
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants