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

Support for Import Maps #49443

Open
wesleytodd opened this issue Jan 24, 2020 · 122 comments
Open

Support for Import Maps #49443

wesleytodd opened this issue Jan 24, 2020 · 122 comments
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. feature request Issues that request new features to be added to Node.js. never-stale Mark issue so that it is never considered stale

Comments

@wesleytodd
Copy link
Member

wesleytodd commented Jan 24, 2020

I was hoping to start the conversation around getting Import Map support into core. I figured starting here with a proposal was better than starting right off with a PR.

What Are Import Maps

This proposal allows control over what URLs get fetched by JavaScript import statements and import() expressions. This allows "bare import specifiers", such as import moment from "moment", to work.

The proposal was mainly targeted at browsers where URL imports have many downsides without bare specifier support. It enables a standardized map file format to be used when resolving import specifiers, giving developers more control of the behavior of import statements.

Here is an example of the format:

{
  "imports": {
    "todayis": "node_modules/todayis/index.js"
  },
  "scopes": {
    "node_modules/todayis": {
      "english-days": "node_modules/english-days/index.json"
    }
  }
}

Proposal link: https://github.com/WICG/import-maps

Why should they be supported in Node.js?

Currently we use the structure of the filesystem and a resolution algorithm to map import specifiers to files. These files are typically installed by a package manager in a structure which the Node.js resolution algorithm knows how to resolve. This has some drawbacks:

  1. It is slow
  2. It can lead to unexpectedly resolving modules
    • if you have a node_modules at your filesystem root for example
    • if your package manager has hoisted a transitive dep and you access it from the top level
  3. Tools are required to implement copies of the Node.js resolution logic
  4. Users and package managers jump through hoops to try and make the filesystem performant

If we had import maps we could circumvent most of this. Package mangers can generate an import map, enabling perf improvements and simplicity in their implementations. It increases startup performance of apps because they don't have to do as much filesystem access.

One interesting example of what import maps enables is filesystem structure independent workspaces (think yarn workspaces but without the requirement of the modules being under the same root directory).

Implementation

As an example implementation I created this gist:

https://gist.github.com/wesleytodd/4399b2351c59438db19a8ffb1f3fcdca

To run it:

$ git clone [email protected]:4399b2351c59438db19a8ffb1f3fcdca.git hello-today
$ cd hello-today
$ npm it

This uses an experimental loader which loads an importmap.json and falls back to the original filesystem lookup if it fails. I am also pretty sure this very naive implementation is missing edge cases, but in these simple cases it appears to work.

Obviously for support in core we would not use a loader, but I think the rest would be similar. Open questions I have are:

Where would node find the importmap.json?

My first idea is at node_modules/importmap.json. I think this would be expected behavior from users if their package managers started writing this file.

How would users override the importmap.json?

For this I think a reasonable approach would be a cli flag (--importmap) and a package.json ("importmap": "./importmap.json") field.

Do we want to wait for browsers to standardize import maps?

#49443

I think that this is a very valid concern. Is there a way to help push that forward? The benefits are pretty large in node IMO, and it would superseded all the work currently being done on yarn pnp and npm 8. If we let those go forward but then this spec lands it might be even worse for shifting the community.

Could we release it as flagged & experimental until browsers standardize? This would mean users could opt in, but as it changed we could follow along.


Thoughts? Next steps?

@ljharb
Copy link
Member

ljharb commented Jan 24, 2020

Since import maps seem far from being finalized or standardized in browsers, it feels like it’d be best to confine it to userland loaders for the time being.

@wesleytodd
Copy link
Member Author

wesleytodd commented Jan 24, 2020

@ljharb Added an open question at the end with my concern:

it would superseded all the work currently being done on yarn pnp and npm 8. If we let those go forward but then this spec lands it might be even worse for shifting the community.

@ljharb
Copy link
Member

ljharb commented Jan 24, 2020

I don’t think it would supersede it - they’d just get to provide an import map instead of their current approach, whenever it became available. Perhaps @arcanis can weigh in here.

@jkrems
Copy link
Contributor

jkrems commented Jan 24, 2020

The benefits are pretty large in node IMO, and it would superseded all the work currently being done on yarn pnp and npm 8. If we let those go forward but then this spec lands it might be even worse for shifting the community.

I assume that import-map.json would be written by package managers (if they choose to support it). Both yarn and npm have already somewhat committed to a public interface for their resolution structure. Without strong signals from their side that they'd realistically move to a shared metadata format, I don't see them making the switch. pnp.js would stick around anyhow. And require-style resolution would still be needed for backwards compatibility. So they would need two files instead of the one they already have.

That being said: Having a well-supported loader that uses an import map would be neat to have. But I agree with @ljharb that at this point it doesn't really belong into node core. There are some pretty major open questions with import maps in node as well. E.g. your example lays out relative file URLs. One major development in more recent package layouts was that the files do not actually live in node_modules/ (if possible). And in that world, a stable (version controlled) import-map.json can't (?) really use actual file URLs afaict.

@wesleytodd
Copy link
Member Author

wesleytodd commented Jan 24, 2020

I don’t think it would supersede it - they’d just get to provide an import map instead of their current approach, whenever it became available. Perhaps @arcanis can weigh in here.

"Instead" is superseding it. Users will couple to what the package managers release, and it will not be a simple matter to just replace it. This could be a large hurdle to overcome in the long run.

Both yarn and npm have already somewhat committed to a public interface for their resolution structure. Without strong signals from their side that they'd realistically move to a shared metadata format, I don't see them making the switch.

I agree we would want strong buy in from the package managers. I presented a PoC of workspaces built on top of this to @ruyadorno today and he is going to show it internally at npm. I would love to also hear what @arcanis has to say on this.

pnp.js would stick around anyhow.

This is part of the problem I hope we could solve here. If node core supported a shared format we wouldn't fragment the ecosystem with package managers providing their own loaders. It is much better IMO to have them build to a shared spec the runtime understands.

One major development in more recent package layouts was that the files do not actually live in node_modules/ (if possible).

Import maps specifically solve this problem. They can point anywhere on the filesystem. In the workspace example I have (not oss yet) I am working on moving them into a shared workspace cache.

And in that world, a stable (version controlled) import-map.json can't (?) really use actual file URLs afaict.

In the global shared cache scenario it would not be portable. The example I showed using relative urls to make it portable, but my workspace PoC uses full paths. I think it is reasonable to support both.

@guybedford
Copy link
Contributor

Amazing to see this work, thanks @wesleytodd for investigating.

@ljharb would you disagreement over stability be mitigated by flagging such an implementation. Personally I see no harm in experimentation in core, rather than pushing out experimentation here entirely to userland. It shows a willingness for core to cooperate with projects exploring this space over forcing completely separate approached entirely leading to the fragmentation @wesleytodd mentions.

It may well work out that we find that projects do need their own full import maps implementations, in which case Node.js can always deprecate such an experimental flag based for that outcome.

As @jkrems has touched on, one of the issues here is that import maps as a replacement for the entire resolution system is tricky due to these details. For example, with conditional exports, the same package can resolve different depending on environment information. Import maps don't have this same functionality and likely won't... rather they can be considered an output of an environment function at some level - an import map specific to the browser is different to an import map specific to Node.js.

In this way I prefer to think think of import maps somewhat like "ephemeral views" into the project, representing a caching / precomputation of the resolution operation. They have a locking property, but that should not be confused with lock files, which are about install reproducibility, something that is clearly a superset of the features import maps provide.

Thinking about the user features, I see these as goals of import maps in Node.js, when they can be allowed to be complementary to the Node.js resolver:

  • Mocking (map just the mocked URLs)
  • Vendoring (map just the vendored packages)
  • Resolution inlining for performance / encapsulation from FS / security (mapping everything)

I do see having import maps as being the default and only form of resolution in Node.js as a non-goal here though. Rather an approach where the import map can allow intercepting the resolution and then having that work with userland approaches to explore the above space seems most beneficial to me.

@wesleytodd
Copy link
Member Author

wesleytodd commented Jan 24, 2020

Amazing to see this work, thanks @wesleytodd for investigating.

Thanks for responding on twitter and sending me down this path :)

For example, with conditional exports, the same package can resolve different depending on environment information.

I think this is also solvable inside import maps. The spec doesn't really specify what the scope can be used for, so I see no reason that information cannot be encoded into the scope mapped from the conditional exports. Either that or the tooling would generate a separate environment import map (as you mention).

They have a locking property, but that should not be confused with lock files, which are about install reproducibility, something that is clearly a superset of the features import maps provide.

In the PoC I have not shared yet I actually use npm (@npmcli/arborist technically) to generate the tree and then the import map. This means that it respects the lock file, and will reproduce the same import map from the same lockfile.

I do see having import maps as being the default and only form of resolution in Node.js as a non-goal here though.

Agreed, and thus my example falling back to the existing file system based resolution. I think this would be a very important key feature to keep.

@coreyfarrell
Copy link
Member

Could we release it as flagged & experimental until browsers standardize? This would mean users could opt in, but as it changed we could follow along.

Does --experimental-loader make this possible already? For testers npm i wesleytodd/import-map-loader would be easier than rebuilding node.js.

In the PoC I have not shared yet I actually use npm to generate the tree and then the import map. This means that it respects the lock file, and will reproduce the same import map from the lockfile.

I'm interested in how large the generated import maps would be. The node_modules of one of my current projects contains 576 modules and 6355 .js files. Does every .js file get listed in the import-map? At what point does the load/parse of a large import-map exceed the current resolver overhead for loading only a couple files from node_modules? I'm also interested in the potential memory overhead of keeping a large import map loaded.

@wesleytodd
Copy link
Member Author

wesleytodd commented Jan 24, 2020

Does --experimental-loader make this possible already? For testers npm i wesleytodd/import-map-loader would be easier than rebuilding node.js.

That is really how I am testing it today. If you check the test script in the package json of the example you see it passes --experimental-modules --loader ./loader.mjs

Does every .js file get listed in the import-map?

No. It only lists packages then falls back to file system imports for the rest. Although that was a decision I made, which you could change for many interesting reasons (as Guy mentioned, mocking).

At what point does the load/parse of a large import-map exceed the current resolver overhead for loading only a couple files from node_modules?

My guess? Never. That said we should test it. Also, it would be really simple to prune the import maps based on static analysis if the app entry point does not load anything from that module. It would also make sense that the package managers would generate a --production which would only be the production dependencies.

I'm also interested in the potential memory overhead of keeping a large import map loaded.

I also am guessing here, but not much. I will try this out on the largest projects I can find at work and report back on the size. Also, we can easily optimize the the memory footprint from the import map format to something better at runtime then just throw away the json format.

@coreyfarrell
Copy link
Member

Does --experimental-loader make this possible already? For testers npm i wesleytodd/import-map-loader would be easier than rebuilding node.js.

That is really how I am testing it today. If you check the test script in the package json of the example you see it passes --experimental-modules --loader ./loader.mjs

Ah not sure how I missed the example link. I'll check that out as soon as I have a chance.

It only lists packages then falls back to file system imports for the rest. Although that was a decision I made, which you could change for many interesting reasons (as Guy mentioned, mocking).

If the import-map is has module granularity (not listing every file within) then this greatly reduces my performance/memory concerns.

At what point does the load/parse of a large import-map exceed the current resolver overhead for loading only a couple files from node_modules?

My guess? Never. That said we should test it. Also, it would be really simple to prune the import maps based on static analysis if the app entry point does not load anything from that module. It would also make sense that the package managers would generate a --production which would only be the production dependencies.

Static analysis sounds difficult if possible, especially on the development side. I'm thinking of babel plugins as a perfect example, without specific knowledge of babel you cannot know that {presets: ['@babel/env']} in babel.config.cjs means that @babel/preset-env is loaded.

@ljharb
Copy link
Member

ljharb commented Jan 24, 2020

@guybedford flagging it is better, for sure, but i take it as a given that it'd start out flagged. My main concern is that unless import maps stabilizes, is standardized in browsers, and also is usable for node without doing backflips, then i'd hope node core would never ship any form of import maps - put more positively, when those things happen, node core would obviously ship import maps! Given the large uncertainty, it seems premature to me to even signal a possible future core implementation when there may be none at all.

@arcanis
Copy link
Contributor

arcanis commented Jan 24, 2020

One interesting example of what import maps enables is filesystem structure independent workspaces (think yarn workspaces but without the requirement of the modules being under the same root directory).

The problem behind disjoint workspaces isn't in term of resolution - that's fairly simple to implement - but rather developer experience. For example, if you run yarn add foo into the disjoint workspace, how does Yarn knows that it needs to update the lockfile from a completely different folder¹?

¹ It's a rhetorical question; there are ways, but they are all less good that just telling our users to use link: or portal: to add their disjoint workspaces as external dependencies. Not everything has to be in the core 🙂

it would superseded all the work currently being done on yarn pnp and npm 8. If we let those go forward but then this spec lands it might be even worse for shifting the community.

I don't think it would. It's not even clear whether import maps would have all the features we would need. For example, Yarn is able to throw semantic exception when a package isn't reachable, explaining why the package cannot be found. It's quite harder for import maps, which don't have package-level semantics - just folder-level semantics:

Something that got detected as your top-level application (because it doesn't seem to belong to any package) tried to access a peer dependency; this isn't allowed as the peer dependency cannot be provided by any parent package

Required package: react (via "react")
Required by: frontend/app.js

I agree we would want strong buy in from the package managers. I presented a PoC of workspaces built on top of this to @ruyadorno today and he is going to show it internally at npm. I would love to also hear what @arcanis has to say on this.

I think my main question is: what do you think this would improve? I know you mentioned you would find it cleaner, but that's subjective. I personally find a JS API clearer than a file format, for example. So in concrete terms, what would it bring to the table?

  • Compatibility would be the most obvious place where it could bring some wins. At the same time, even with the node_modules system being more or less universal, everyone still ends up reimplementing the Node resolution somehow. This to me signals more a lack of good API that the need for another universal format.

  • Feature set is definitely not one - for obvious reasons loaders are able to do everything the import maps can do, plus more.

  • Cleanliness is maybe one, but again that's very much open to debate and I don't think there's a good or bad opinion on this one. There are some parts in PnP that I find a bit messy myself (mostly the filesystem overlay), but those aren't the ones that the import map would remove.

  • Performance could be one, but I don't know how true that it, and it could easily go in both ways. For example Yarn currently has this "fallback" feature, where an undeclared dependency can still be accessed if listed by the top-level package (because it's the only safe and unambiguous case). Implementing that in import maps would require to duplicate the top-level dependencies into each and every package record. I don't know how efficient this would be.

    • Additionally, since PnP is already doing static resolution, there is very little performance gain to have by using import maps instead. We already don't need to query the filesystem, except for the extension part. If something needs to be improved in this regard, I'd suggest to rather focus on making file extensions required ... but that's another battle 😛

Overall I'm just not convinced this is the most impactful place to put energy on. I'd rather talk about making the fs module extendable in a proper way, for example.

@bmeck
Copy link
Member

bmeck commented Jan 24, 2020

So, we already have this feature of static resolution mappings to some extent using https://nodejs.org/api/policy.html#policy_dependency_redirection ; in all honesty, even though I wrote the feature and have spent some time tuning performance, the benchmarks there are not pleasant when you run them on medium sized apps particularly regarding startup time. I think we should be more critical about the benefits here as it is not a sure win in terms of performance to merely have static resolution.

If we could also address the other concerns about the limitations of only having data for resolution and adding additional meta-data that seems fine, but starts to seem to just be working towards a different policy file.

@wesleytodd
Copy link
Member Author

Given the large uncertainty, it seems premature to me to even signal a possible future core implementation when there may be none at all.

Where can we look to help provide more certainty? Is it just a matter of "we cannot do it unless the browsers ship it first"? I obviously do not want to repeat the mistakes of the past, but this seems like a hard line to take long term.

but rather developer experience

I think tooling DX is a separate concern. I think the goal of this as a core api would be to enable experimentation in user land on top for what DX should look like. The goal of this proposal is to focus on if this is a good idea for inclusion into core.

they are all less good that just telling our users to use link: or portal:

Two concepts which are exclusive to yarn, made up by yarn, and so not a solution in this context unless you seek to standardize them. This only continues to widen the chasm between package managers, and increase the barriers to entry for another package manager.

For example, Yarn is able to throw semantic exception when a package isn't reachable

IMO, this could be a good addition to core, It bridges the gap and brings the whole community up together. If there are hints or metadata which pacakge managers can give to the runtime to improve in this way it would be great! We should look for a loader api which enables this, and I believe that import maps could be that api.

I personally find a JS API clearer than a file format, for example.

I am fine with it being a js api. My internal workspace PoC is that, it follows more closely the approach in PnP, but uses an import map as the structured input format.

This to me signals more a lack of good API that the need for another universal format.

Completely agree! This paragraph this sentence is in is exactly my goal here.

loaders are able to do everything the import maps can do, plus more.

Do you mean this spec? https://whatwg.github.io/loader/

we already have this feature of static resolution mappings

If we could also address the other concerns about the limitations of only having data for resolution and adding additional meta-data that seems fine, but starts to seem to just be working towards a different policy file.

This is close but not equivalent right? Also I think the use case of security concerns remapping modules is a bit different scope than import maps. I actually think that import maps with the features in the policy api would be a better way to implement what is being done because it would be more generic and more approachable by users.

the benchmarks there are not pleasant when you run them on medium sized apps particularly regarding startup time

Do you have links for this? I would love to look at them. I admit that I have done no benchmarking in this regard, and it for sure would have to be done.

@bmeck
Copy link
Member

bmeck commented Jan 24, 2020

@wesleytodd see stuff like #29527 which would add a benchmark to node specifically about that feature.

@GeoffreyBooth
Copy link
Member

I would be fine with creating a new flag to experiment on supporting import maps. If it becomes a standard, Node should support it.

I agree that there are questions of how useful import maps really would be to Node. The new package.json "exports" implements a lot of the same functionality, with additional features; and we designed "exports" to be easily convertible by tools into import maps. That said, Node should absolutely support any standards; and we would be in a stronger position to influence the standard as it develops if we have our own flagged implementation for folks to compare against the browsers’ flagged implementations. If import maps never end up getting standardized, we can just remove ours.

@wesleytodd
Copy link
Member Author

wesleytodd commented Jan 24, 2020

The new package.json "exports" implements a lot of the same functionality,

It is a bit different. The goal of an import map is that tooling can do creative things with where the modules live, which is different than what the exports. Right?

we would be in a stronger position to influence the standard as it develops if we have our own flagged implementation

Strong 👍 on this! I think that waiting on browser vendors is a passive approach. Having a flagged feature which gets good usage is a strong signal to the browsers that this implementation works. It also gives us a better seat at the table in the discussions because we will have experience building and maintaining it.

@bmeck
Copy link
Member

bmeck commented Jan 24, 2020

That said, Node should absolutely support any standards

I'd be against stating just because something is a standard it is good to land it everywhere. The implications of import maps and the integrations across environments does not seem to be well discussed. If we saw some interest in import maps beyond the current browser heavy path forward to address things like those in this issue such as having more data, composition on a package level, etc. I'd be more interested in that specific feature. As it stands things, Node is not beholden to follow any given standard it does not feel is a good fit for the runtime; and, as it stands I do not see import maps as being something that is well suited towards being merged. Though, I would state I am generally biased against WHATWG controlled features in general these days and more so biased against them the more interactions I've had over the years so it isn't like I'm a neutral party here.

@wesleytodd
Copy link
Member Author

If we saw some interest in import maps beyond the current browser heavy path forward to address things like those in this issue such as having more data, composition on a package level, etc. I'd be more interested in that specific feature.

More interest from whom?

As it stands things, Node is not beholden to follow any given standard it does not feel is a good fit for the runtime

I was hoping to make that case, are there specific things you think I could expand upon above which would better make the case for you? I have mentioned that I have a PoC of one key feature it has that current solutions do not provide. If I provided some more use case examples than my very simple gist would that help?

biased against WHATWG controlled features

People matter, and if you don't have faith in their process is there something we could do to move the specification into a better home?

@bmeck
Copy link
Member

bmeck commented Jan 24, 2020

More interest from whom?

People wanting to implement import maps in other environments <-> the import maps proponents. As of yet, we have some interactions about how other environments may use what the web wishes to use, but not about what the web can allow to let other environments have their own use cases.

I was hoping to make that case, are there specific things you think I could expand upon above which would better make the case for you? I have mentioned that I have a PoC of one key feature it has that current solutions do not provide. If I provided some more use case examples than my very simple gist would that help?

One of the big things is composition being discussed at length. E.G. Placing something under node_modules/ means that the location in which a program is being run affects its import map. Since multiple node_modules/ folders may exist (or none) this means discussion of how multiple import maps needs to take place and likely cannot be put off like in the browser.

Other data such as integrity strings would also be of benefit to discuss. I don't think the path resolution is the only thing that will be used when doing imports. In the future, browsers are also looking at requiring module attributes and those likely also need to be discussed.

I think providing use cases that elaborate on comparisons with existing Node features and explanation of why import maps are a better fit than a Node specific alternative would be helpful for these kinds of things.

People matter, and if you don't have faith in their process is there something we could do to move the specification into a better home?

This is part of the issue, things like RFCs have sometimes been taken over and superseded by WHATWG claiming to be the source of truth rather than the original RFC. I doubt moving the specification would prevent such behavior in the future.

@wesleytodd
Copy link
Member Author

wesleytodd commented Jan 24, 2020

One of the big things is composition being discussed at length

I have read through most of the conversation on this. I agree the direction that conversation takes could effect our implementation. That said, I think the only thing which makes sense for node here is to have one import map and no composition. In the browser this doesn't work, and I understand that. Is there a reason we could not implement only a subset and not support composition ever in the future? Having a dep define an import map in node has issues, it would have large security concerns and usability concerns.

Other data such as integrity strings would also be of benefit to discuss

Agreed! Digging into your policy api stuff I really think there is overlap here we would want to resolve.

I doubt moving the specification would prevent such behavior in the future.

Is the alternative to write a separate rfc with the same work? I really prefer to avoid conflicts or politics where possible lol. What could we do to get the equivalent functionality while avoiding this issue?

@bmeck
Copy link
Member

bmeck commented Jan 24, 2020

What could we do to get the equivalent functionality while avoiding this issue?

I do not have an answer to that question due to the above behavior of overtaking anything we might propose as separate.

@GeoffreyBooth
Copy link
Member

FYI, Deno supports import maps: https://deno.land/std/manual.md#import-maps

@jkrems
Copy link
Contributor

jkrems commented Feb 26, 2020

Since it has been brought up recently, my current take is: It doesn't sound like there have been any signals that npm and/or yarn would adopt it. And without that, the case doesn't seem all too convincing right now. Even on the browser side, Chrome seems to be the only party more or less actively investing into import maps.

There's also some pretty fundamental questions:

  1. Can we support import maps in CommonJS? If not, are we okay with that?
  2. How would import maps work with conditional exports? Would we support having multiple import maps? Would we extend the import map into something that wouldn't actually match the official spec?

It feels like node support for import maps is fairly far down the list of "things that block people from using import maps". So I'm not sure it's a good time to invest into adding it to node.

FYI, Deno supports import maps

Thanks, that's good to know! But it's worth noting that they're running into the exact problem that I'm afraid of for node: They added support almost a year ago and there's still no good story for actually using import maps (see denoland/deno#3585).

I think that's a good example of why I think that native support in node isn't the most important step if the goal is to make import maps in node viable.

@wesleytodd
Copy link
Member Author

wesleytodd commented Feb 26, 2020

It doesn't sound like there have been any signals that npm and/or yarn would adopt it

I can work on this. That said, I do not feel that this should be part of the acceptance criteria. Other tooling can fill in here if they are not interested.

Can we support import maps in CommonJS? If not, are we okay with that?

Yes. My proof of concept, while using a ESM loader works on CommonJS.

How would import maps work with conditional exports?

I think the tool generating the map needs to know the target. I haven't fully thought through this design, and we would for sure want a clear story on this. I think avoiding making any changes to the spec without also working with them to land those changes in the spec is a bad idea, but I think it just works as spec'd today.

It feels like node support for import maps is fairly far down the list of "things that block people from using import maps". So I'm not sure it's a good time to invest into adding it to node.

I think before PnP and tink like functionality, which heavily overlap with this proposal, gain large scale adoption is the perfect time. Otherwise we are just fighting against the current.

They added support almost a year ago and there's still no good story for actually using import maps

It sounds like the issue they have is specifying the flag. With a loader we have the same problem, but my proposal is to give a standard location which will be loaded by default. Does this not circumvent the issues described in that deno issue?

Also separately they are coping with the design decision to import from URLs at runtime. Things which I strongly advise against, and something which (from my position at Netflix) will be a huge concern if Node were to adopt.

@jkrems
Copy link
Contributor

jkrems commented Feb 26, 2020

Yes. My proof of concept, while using a ESM loader works on CommonJS.

Right now we don't support custom loader hooks in CommonJS. And if this becomes a built-in feature in node, I assume we'd have to implement it in the actual require loader. There's no call from the require loader into the import loader for resolution. E.g. CommonJS does recursive searches that aren't really how import maps work. There'd have to be some statement on expectations for this.

but I think it just works as spec'd today.

I'm pretty sure it doesn't, the spec removed fallbacks afaik. So I don't think we could generate a valid import map with conditional resolution. Happy to be proven wrong but that's my current understanding.

I think the tool generating the map needs to know the target.

Conditional exports do allow for different conditions at runtime. E.g. import vs. require or a speculative private/internal condition.

I think before PnP and tink like functionality, which heavily overlap with this proposal, gain large scale adoption is the perfect time. Otherwise we are just fighting against the current.

Since both of them have working implementations/designs that support CommonJS fully - why do we think that import map support in node would change the adoption probability? I don't think end users care if they pass --import-map=tink.modulemap.json or -r tink. Especially if PnP/tink end up as loaders and may even be specified in package.json one day.

@wesleytodd
Copy link
Member Author

Right now we don't support custom loader hooks in CommonJS.

Sorry, I was unclear here. My POC uses the existing --loader feature, not loader hooks. I agree we would need support in both contexts.

I'm pretty sure it doesn't, the spec removed fallbacks afaik.

I was not aware the specification had an opinion here. Do you have links on this?

Conditional exports do allow for different conditions at runtime. E.g. import vs. require or a speculative private/internal condition.

I will look into how this could work. My first naive idea is that the scope is the right place for this logic to live. If the loader did the same logic it would do based on the package.json field but inside the import map, seems like a scope could look like /path/to/scope::import. Seems like that is inline with the intent of the spec no?

why do we think that import map support in node would change the adoption probability?

I think the fact that they both have low popularity solutions for this is a sign that a common shared core belongs in node. I think that both should be able to build on top of the same base, instead of the fragmentation which is happening currently. If there is another better proposal for a common core, I am fine going in that direction.

Maybe I could see package installers providing loaders as an option, but that is just even stronger fragmentation. With the current direction it seems to be leading to incompatible ecosystems, I would hate to see one of the strongest parts of the node ecosystem broken because we fail to set a shared direction on this. I would rather have an imperfect solution which aligns people with compatibility in mind, than a perfect solution in 5 years when everything has fragmented off.

@GeoffreyBooth GeoffreyBooth transferred this issue from nodejs/modules Sep 1, 2023
@GeoffreyBooth GeoffreyBooth added feature request Issues that request new features to be added to Node.js. esm Issues and PRs related to the ECMAScript Modules implementation. labels Sep 1, 2023
@github-project-automation github-project-automation bot moved this to Pending Triage in Node.js feature requests Sep 1, 2023
@al6x

This comment has been minimized.

@trusktr
Copy link
Contributor

trusktr commented Oct 10, 2023

FYi, <script type="importmap"> is released natively in all browsers now! Plus in Bun and Deno. Perhaps time to revisit this.

It will be sweet to have package management tooling that works the same way regardless if writing for server or client, regardless of which runtime or browser.

@GeoffreyBooth
Copy link
Member

It’s at the top of the loaders to-do list. Pull requests are welcome. If you start working on a branch, please let us know at https://openjs-foundation.slack.com/archives/C053UCCP940.

Copy link
Contributor

github-actions bot commented Apr 8, 2024

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Apr 8, 2024
@joeljeske
Copy link

This is still relevant

@wesleytodd
Copy link
Member Author

Hey @joeljeske, when you drop into a thread like this please try to provide a bit more context on why you are asking and what your goal is. As it stands, I would likely mark your comment as off-topic or spam even. The work is still underway over in #50590 if that is what you mean.

@ruyadorno
Copy link
Member

@wesleytodd I believe @joeljeske might have just wanted to make sure the issue remains open. I'll go ahead and remove the stale label.

@ruyadorno ruyadorno removed the stale label Apr 8, 2024
@wesleytodd
Copy link
Member Author

FYI, I asked in the OpenJS Slack about this bot. I am happy to open an issue (or maybe even a PR if it is a fast enough change) to improve the language. Honestly I am not even sure if this issue should remain open since there is a PR now, but if I am going to close it I would like to also improve the message that "it is unlikely to be implemented" which is not correct and too overbearing for a both to make that claim.

@joeljeske
Copy link

joeljeske commented Apr 8, 2024

Yes, thank you. I was wanting to deactivate the stale bot. I should have also made my own language more clear. My apologies.

Thank you for your continued work on this effort!

Copy link
Contributor

github-actions bot commented Oct 6, 2024

There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-stale Mark issue so that it is never considered stale label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Oct 6, 2024
@guybedford guybedford added never-stale Mark issue so that it is never considered stale and removed stale labels Oct 6, 2024
@guybedford
Copy link
Contributor

guybedford commented Oct 6, 2024

I've added the never stale here, as I don't know of any resistance to implementing this feature in Node.js, other than just getting the PR to a mergeable state. Happy to be corrected on this though if there are other perspectives here.

@wesleytodd
Copy link
Member Author

I have had a few conversations recently about working with a few other folks who have time and are able to help. It is nearly over the finish line, and if I had any dedicated time to work on it since last year I would have loved to finish it. If no one makes a serious attempt to work with me on this before Jan, I hope to have that time to finish it up then. So not sure what stale does, but I would prefer if it was not closed.

@jkrems
Copy link
Contributor

jkrems commented Nov 12, 2024

Would the idea be that conditions are resolved at package installation time? Or would we use an "extended" import map that supports conditions and potentially also wildcard patterns to reduce the size / IO required to generate?

@wesleytodd
Copy link
Member Author

Would the idea be that conditions are resolved at package installation time?

Can you clarify your question? Are you suggesting non-standard additions? If so, I think we should keep that discussion separate from this PR which is intended as only the base level of support for the whatwg spec. If not, which is entirely likely since I am not clear on the question, can you help me better understand your goals?

@jkrems
Copy link
Contributor

jkrems commented Nov 12, 2024

@wesleytodd Basically, if I am a package manager and want to generate an import map on install, I would have to handle packages that conditionally export different things. For example:

{
  "name": "pkg",
  "type": "module",
  "exports": {
    "node-addons": "./addon.node",
    "default": "./slow.js"
  }
}

In the import map, there can only be one entry for "pkg" but without knowing the --no-addons flag passed to node at runtime, the expected resolution is ambiguous. This gets even more complicated if you consider use cases that pass custom conditions.

Just wanted to bring this up as a "what does it mean to support import maps", in terms of support for existing packages. The answer could absolutely be "this becomes a package manager/outside tooling problem and node stops caring about conditionals". And this doesn't have to stop a PR to land raw browser-style import maps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. feature request Issues that request new features to be added to Node.js. never-stale Mark issue so that it is never considered stale
Projects
Status: Awaiting Triage
Development

No branches or pull requests