The ESM resolver does not respect package mains for packages imported via relative imports #41940
Description
Activity
I suspect this was expected/intended, but it's also likely that the (quite rare, I'd think, but certainly long and widely discouraged) use case of vendoring dependencies wasn't considered.
Yeah, it's just odd that you can move around an old cjs package and vendor it and as long as you only try to use it from cjs, it works fine, but once you try to pull it into a newer esm mode file, it fails. :(
I agree; there's a ton of things the CJS resolver can do that it's odd that the ESM resolver can't do.
Yes this was discussed in detail, on the topic of directory imports, and was an explicit decision alongside extension searching. Vendoring of depenencies is still handled by monorepo symlinking or the package.json local dependencies pattern:
{ "dependencies": { "vendored": "file:../package-path" } }where npm will symlink into node_modules in the above scenario, enabling the local package import with deduplication via realpathing.
But, like, the resolver still pulls other stuff from the package.json (like its format), it clearly knows it's there; it just doesn't respect the entrypoint. 😦
It makes it actually hard to develop (esm) packages outside a node_modules structure, and requires the complicated monorepo-tool-symlinking-nonsense. 😅
The simplification was that relative resolution can always be done without needing any lookups, it's just bare specifier resolution that runs through package resolution rules. This was part of removing the "magic" of resolution. I'm sorry if you feel it's unmagical now!
Just like extension searching, what seems very simple can lead to cascading statting:
/package.json
{ "main": "./app" }/app/package.json
{ "main": "./subapp" }/app/subapp/package.json
{ "main": "./main-finally" }I'm hopeful for Windows symlink dev workflows going forward... perhaps other ways of defining custom package mappings for Node.js could be useful too (this being a separate thing to import maps / module mappings).
@weswigham for users that this causes upgrade issues for, note that --experimental-specifier-resolution=node should still provide the old lookup behaviour as well.
I mean, sure, but it's more like the esm resolver isn't respecting the intent of the package as written? Like, that's great and all, but if I'm vendor'ing someone else's package, I'm not looking to be in the business of manually resolving their entrypoints, y'know?
Is there more info on this kind of vendoring? I assume it only works if the package happens to have no dependencies because a non-node_modules vendor directory would mean that cross-package imports using bare specifiers break?
Normally this kind of vendor'ing is used for older browser umd-type packages which don't normally have external dependencies, yeah (and brings up anther reason why people may vendor in the first place: to host some part of their js files on something like gh pages without hosting/comitting the rest of their node_modules files used in their build).
It's an issue I encountered here in core, where I needed to import from test/doctool packages installed in tools/doc/node_modules. The only workaround I found was to re-export them from a file in tools/doc:
node/test/doctool/test-doctool-json.mjs
Lines 8 to 11 in 863d13c
https://github.com/nodejs/node/blob/master/tools/doc/deps.mjsReacted by Wesley Wighami remember this from ye olden modules WG meetings where only during bare specifiers would searching potentially occur, this lets the resolver be in step with WHATWG's resolve a module specifier step in https://html.spec.whatwg.org/multipage/webappapis.html#hostresolveimportedmodule(referencingscriptormodule,-modulerequest):resolve-a-module-specifier
I'd also think another workaround is to use bundledDependencies for vendoring as well : https://docs.npmjs.com/cli/v8/configuring-npm/package-json#bundleddependencies
Per changing the behavior / changing so this is expected to work I think it would be complex since now every specifier might require main and exports to be processed instead of just bare specifiers. I'm open to discussion on this, but it was previously unable to be agreed that it would be a desired behavior. In particular, I'm a bit concerned about:
/package.json /foo/entrypoint.mjs # imports ../bar/... /bar/package.json
If this skips over /package.json's exports and only used /bar/package.json's fields. Right now exports also doesn't work ever to my knowledge using relative specifiers and it would be my main concern as people start adopting things. I'm not saying any of this is fatal, just a more than trivial discussion has already occurred and would be required I'd imagine to change things again.
I mean, we're doing that here, right? I, at least, came here to file this because I found the current behavior surprising, given my experience with node packages in the past - beyond the 'imports usually require extensions' thing which can be seen as a simple choice of style enforcement from a user perspective.
It seems like a few others have run into similar issues since shipping the modules mvp; as-is, the es module implementation in node feels kinda... Tacked on? There's a bunch of rough spots (almost anywhere the existing module ecosystem is acknowledged), and I won't claim this is the worst one, but it is one that seems like something that could be fixed so long as the philosophical "relative imports shouldn't be redirectable" is just that - philosophical. And, like, I'm positive someone will pop in and say "well, a user loader could respect package.json files on relative imports if that's important", and my point is moreso that it's surprising that the default loader behavior doesn't. They're node packages, after all (often written in cjs for older versions of node), and node isn't recognizing them (sometimes, in some ways). The author of the package, and their intent, is being ignored, despite being known.
@weswigham I'm not making a statement on what we should do but I think the idea that package.json is being ignored in various places on various fields is true already with exports and imports. Having the behavior differ is a difference certainly, but require continues to work for older code written for older versions of node. The discussion is about having import change to do both searching for package.json#main and searching for the file since plenty of main field values do not have extensions. If this broke require somehow I'd be 100% onboard with a change, but this inconsistency would still be inconsistent if we add extension searching (off require.extensions?) etc. in this case. I'm not 100% convinced it should be changed right now since we at least have some level of decision previously.
Reading the TS issue, it looks like the bug was with a bare specifier so jumping to relative specifiers needing to be fixed seems a bit confusing of a leap?
13 remaining items
There are a lot of complaints about ESM lacking the lookup features of CJS - it's just that when people run into them, they simply stick with ESM transpiled to CJS.
The fact that we're living in a dual CJS/ESM world doesn't mean that ESM has to conform to CJS-s world view
Nor does the fact that browsers implement ESM one way mean that node's ESM has to conform to a "no build step" world view.
But, like, reading a directory's (cjs) package isn't going to stop anyone from only using esm? If you're trying to use only esm in node, you're already purging all cjs from your dependency tree (which is probably nontrivial unless the code was never meant for explicitly just node in the first place). Like, you can already easily be importing a cjs file from a directory which is just as unsupported on web?
Reacted by Jordan HarbandAnd, BTW, if you're going it with package.json, what about index.js inside the folder? That works in CJS, and will break with ESM too.
The origin in the TS issue is a lack of index searching so likely this is needed if we go through with any changes.
Not necessarily index searching in general, but a package file being present implying a default main value of index.js, I'd assume. (Since that's the default entrypoint for both cjs and esm packages)
@weswigham that assumes the main value has a default, which isn't true within node core itself.
I think it's fair to say that we don't have consensus for making any changes in this area. I for one don't want resolution to get any more complicated than it is, if we can help it, and I don't find the use case presented here compelling. It was a design choice with ESM to not replicate all of the magic of CommonJS resolution and I think we're better off for that decision.
Reacted by Gil Tayar and Owen BuckleyReacted by Jordan HarbandI think discussing how to alleviate things while maintaining simplicity and forwards strategy is fine. Right now this looks a bit confusing scope wise to me like I've stated. If scope could be cleared up and more focus on effects of doing this action it would be good. I've brought up a few things in passing glance on the comments here, but things like performance being cast aside is a bit odd since as I recall one of the original push backs against .mjs from TS was exactly because of searching becoming too heavy so it was a discussion about if removing searching would be fine for TS at the time (this was years ago, a full TS team sync might be a pleasant refresher on what the exact details were/are now).
The OP has a very specific, very small scope - relative imports should respect package files in the same way a relative require call (observably) does. Nothing else is required for the OP's ask. Technical whataboutism aside, it's a very straightforward problem statement, that would seem to ask to respect code as it was intended to be used better.
Is this the kind of mire that just needs a concrete implementation to talk about, so we're not talking in circles about hypothetical implementation what-ifs?
Reacted by Jordan Harband and Marvin HagemeisterOff-topic, but probably deserving of a reply.
And, frankly, I haven't heard any grumbling in that area. Nobody's complaining about the lack of support for importing a directory. It's not a problem in the real world, from what I'm seeing. (Of course, that's from my point of view, and the people I'm hearing)
Oh boy howdy you don't read the TS issue tracker. 😁 We get constant (weekly if not daily) requests asking us to make node's new resolution less bad (haven't I said this before?) by doing things like patching over imports and doing the extension and index searching ourselves (something we repeatedly rebuff because we (modern TS development philosophy) desire to reflect and analyze the world as it is, not create our own world). In fact, stuff related to it is some of our most active issues (because we don't lock things unless something heinous has occurred, generally, we just don't post). And I'm positive other tools layered on node resolution have gotten the same feedback (but have differing principles), which is why babel, webpack, rollup, and others all have plugins or settings to patch imports to conform to more old-cjs-resolution-style but emit in a way compatible with modern node resolution. "I haven't seen the complaints" just reads like a kind of willfull blindness to someone triaging those complaints daily. 😭 TS is the front-line here for many users when they encounter analysis pointing out a behavior they don't expect (because it's where the editor squiggles come from), so we absolutely get the brunt of this.
From my perspective, the limited mvp resolution model node esm has is about as far from a "success" as you can get without calling it an abject failure; I'm just obligated to support it.
Reacted by Jordan Harband, Marvin Hagemeister and Matt CaseyI think a PR would help understand what is actually being proposed because my 3 points above remain unclear. Though this just seems like ~ enabling --experimental-module-resolution=node in a specific codepath (which gets its own list of incompatibility issues from time to time). Listing issues that would / would not be avoided by such a PR would be nice if it is some very specific codepath and not generally applicable and the same for ESM code workflows like new URL(specifier, import.meta.url), but overall I actually think to make progress in node core the actual work isn't fixing it for TS since no one really objects to TS having fewer issues but fixing concerns outside of TS.
That said, alleviating the concerns about perf / web compat / consistency across package.json#exports and main / simplicity without calling them irrelevant in various ways and providing paths to solve those would greatly help things. Beyond saying that import maps can do something and explaining how to do something are quite different. For example: import maps explicitly avoid and call out that it is not a good idea to do path searching in them, the bloat affects not just startup time but also the linear search algorithm they use, and policies have experienced similar granularity problems resulting in adding of scopes.
Having these explanations and plans gives forwards strategy to make those concerns have much less of a valid position. I in particular don't see an urgent need if all the solutions for the other claims remain theoretical as well if we get a PR but it would help at least understand scoping of the desired fix, because I for one don't see a solution that is as small scoped as you seem to be saying.
I can pile on another example where this is useful. We use code generation to build out our API input/outputs, and we'd like to be able to use the same generator to build packages to be used in node_modules (as a packaged client) or just dumped into a directory. Having consistent handling of package.json for both scenarios would be very advantageous. Otherwise we have to tell our generator in which context it's being used.
This issue has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.This issue has been automatically closed after 30 days of inactivity following its stale status (no activity for a total of 120 days).
If this is still relevant, feel free to reopen it or leave a comment with additional details so we can continue the discussion.
Version
17.1.0
Platform
Microsoft Windows NT 10.0.19042.0 x64
Subsystem
No response
What steps will reproduce the bug?
It's possible this is intentional (or at least an unintentional effect of how the esm resolver spec was written/implemented), but it seems like a major departure, so here goes. While I was working on fixing this issue in TS, I couldn't shake the feeling that something was off about node's behavior.
Suppose I have a (cjs) package named "pkg":
which I import from my esm consumer package:
everything works fine. Likewise, from my cjs consumer package
everything works fine. In addition, in my cjs package, I can do a relative import of that "pkg" directory:
which works fine, however if I try to do the same in an esm package:
you get an import path error - the package's main is ignored entirely! This means that while in cjs, you can happily move packages out of your node_modules folder (likely to vendor them) and refer to them by relative paths and they keep working just dandy, to do so breaks any esm consumers (even though the package in question is cjs formatted, and so shouldn't care about esm consumers!).
This seems like a fairly major oversight - esm relative imports being unable to load packages entrypoints outside of node_modules which declare themselves to be cjs seems wrong and inconsistent - it's ignoring the declared (or implicit) format of the package the path points to. The package file is still respected for the format for loading any files you write out the whole path to, but is ignored when it comes to calculating the entrypoint.
cc @nodejs/modules
How often does it reproduce? Is there a required condition?
No response
What is the expected behavior?
Importing a folder (import("./a/b")) that refers to a package ("./a/b/package.json" exists) should load that package's entrypoint, as with require.
What do you see instead?
Importing a folder that is a package (import("./a/b") where "./a/b/package.json" exists) ignored the package's entrypoint, unlike require, which respected it.
Additional information
It's possible this is expected, but it actually makes it really hard to work with packages outside node_modules (as when vendor'ing small dependencies), which pre-esm-resolver was actually pretty easy.