resolve icon indicating copy to clipboard operation
resolve copied to clipboard

Support ESM resolution

Open SimenB opened this issue 4 years ago • 37 comments

Hiya! 👋

We've discussed this briefly on Slack, but I thought an issue would be easier to keep track of.

I'm not sure what it would entail, so I'll just list out (i.e. do a brain dump of) the features I think would be needed.

  • Should file be interpreted as CJS or ESM
    • Currently, resolve will only ever return a single string, which is the absolute path to the resolved file. I think in addition to this, we'd need to know if the file should be interpreted as CJS or ESM
    • Related, should resolve taken an option what the caller would prefer? Should it be a static option in, or some callback?
  • Support exports field in package.json
    • Docs: https://nodejs.org/api/esm.html#esm_packages
    • Again, should the caller be able to define which they want? EDIT: Ish, via conditions
  • Support promise for async resolution
    • This is admittedly not a requirement, but it matches the Node APIs making it cleaner to use resolve when implementing custom loaders or using the VM APIs.
    • #210
  • Support URLs. Mostly because import() supports it. It probably makes more sense for resolve to return URLs than an absolute path for ESM?
  • Preserve query strings
    • Makes sense, but just adding it here as a point

There is also a flagged API in Node for this, import.meta.resolve: https://github.com/nodejs/node/pull/31032. Not sure if we should care too much about it, though?

I think that covers it, but you know way more about this subject than I do, so feel free to either close this to open up your own, or edit this OP as you see fit 👍


For background, Jest uses resolve as the default implementation in jest-resolve, although the user can swap it out. I'm currently working on support for ESM natively in Jest, and while we have a version today that sorta works, it's not a compliant implementation. Most of the (known) issues are due to resolution logic. I'd be happy to help implement support here in resolve.

SimenB avatar Apr 25 '20 15:04 SimenB

Some tools/bundlers use resolve('pkgname/package.json') in order to find file locations of package.json files of project dependencies to then extract meta information from these package.json files (like a browser field for file overrides as supported by webpack, rollup etc.). These might break if exports support is added to the resolve package in the same way that only those paths explicitly mentioned in pkg.exports will be resolved.

This came up in https://github.com/react-native-community/cli/issues/1168 where dependencies that don't explicitly export package.json might now be broken. I initially wanted to suggest to use resolve() there instead, but only to realize that resolve() does in fact just not yet have the same problem as require.resolve().

I'm not aware of other problematic tools right now, it's just something we should keep in mind.

See also: https://github.com/nodejs/node/issues/33460

ctavan avatar May 18 '20 21:05 ctavan

The plan is that you'll be able to explicitly request resolve use pre-exports resolution, so i think it's save to use resolve for this use case for the time being.

ljharb avatar May 18 '20 21:05 ljharb

For the "I need to load package.json" use case, why not do something like this:

const {sync: pkgUp} = require('pkg-up');

const packagePath = pkgUp({ cwd: require.resolve('uuid') });

https://github.com/sindresorhus/pkg-up

Could trivially be packaged up into a pkg-of-module and published to npm if people want.

I guess it could be wrong if people have nested package.jsons, but that seems like an edge case we shouldn't care about 😀

SimenB avatar May 19 '20 07:05 SimenB

@SimenB thanks for this suggestion! I took the liberty to copy it over to https://github.com/nodejs/node/issues/33460#issuecomment-630654673 which might be a better place to continue this discussion.

This definitely looks like an elegant solution, but of course it doesn't answer the question yet if the current behavior of node is desired at all 😉 .

ctavan avatar May 19 '20 08:05 ctavan

@SimenB the edge cases very much matter here; we need a way that works in all cases to identify the directory that a package resolves to, and with an "exports" that doesn't expose package.json, or ./, and also has . resolve to a subdirectory, we can't.

I also don't think that's an edge case - that's "every package that uses exports and babel", which is likely to be the 99% case.

ljharb avatar May 19 '20 16:05 ljharb

I don't think I've ever seen a nested package.json. Regardless, this isn't the correct issue to discuss it

SimenB avatar May 19 '20 16:05 SimenB

"type": "module" will make it more common, I suspect, but you're right :-)

ljharb avatar May 19 '20 16:05 ljharb

I don't think I've ever seen a nested package.json

@SimenB does this count as a nested package.json or did you have something else in mind?

NMinhNguyen avatar May 30 '20 01:05 NMinhNguyen

There you go, real usage. 😀 Still, it's off topic for this issue

SimenB avatar Jun 05 '20 10:06 SimenB

If a library adds an "exports" that does not include the "package.json", isn't that a breaking change and conscious decision from the library? It's the same as if the library adds a "files" or .npmignore which ignores some of the files and it breaks some 3rd party usage.

I'm not sure how extended "exports" is already, but might be only a handful of libraries that use "exports" but does not include "package.json" there. All of the examples linked above have in fact already been fixed in the libraries:

franciscop avatar Jun 12 '20 17:06 franciscop

@franciscop yes, it is. Adding "exports" is almost always a breaking change, unless you preserve every possible entry point (use npx ls-exports in your package and you can figure out if you're achieving that or not)

ljharb avatar Jun 12 '20 18:06 ljharb

I noticed @ljharb's comment on Rollup repo mentioning that ESM resolution API is intended to be async only.

Just want to point out that there are use cases for a synchronous implementation.

Mine is: I'm transpiling ESM to CommonJS using Babel, replacing import statements with require(). I'd like to make resolution of import paths compliant with NodeJS. As Babel plugins must execute synchronously, this would require synchronous resolution.

I understand providing both sync and async implementations may be too complicated to be viable, but just wanted to flag that some use cases do exist.

overlookmotel avatar Jan 07 '21 12:01 overlookmotel

node doesn’t have synchronous ESM resolution, because not every ESM specifier is possible to synchronously resolve. I wish this wasn’t the case, but babel needs a way for plugins to run asynchronously anyways, so hopefully this will help encourage that.

ljharb avatar Jan 07 '21 15:01 ljharb

I thought Node's ESM loader has to be async due to top level await, but I wasn't aware of a reason why resolution of specifiers can't be synchronous as it's all static analysis. Am I wrong?

For my use case I'm OK not to support top level await. I'm aware that's not a perfect ESM to CJS translation, but it's close enough.

overlookmotel avatar Jan 07 '21 16:01 overlookmotel

For filesystem specifiers, it can be. However, node wants to preserve the ability to later add, for example, https imports - and thus, so too must resolve.

ljharb avatar Jan 07 '21 17:01 ljharb

@ljharb Thanks for reply. Ah, interesting. I hadn't though of that.

For HTTPS import, would resolution be searching up the folder hierarchy for a package.json file?

i.e. would resolve( './foo.js', { basedir: 'https://www.example.com/' } ) try to fetch https://www.example.com/package.json and parse it?

If not, resolution could be sync - it'd resolve straight away to 'https://www.example.com/foo.js'.

Resolving identifiers within the file would obviously require a network round trip as the contents of the file are needed, but as I understand it, that's outside the scope of this package - it aims only to resolve paths.

Please excuse me if I'm missing something again.

overlookmotel avatar Jan 08 '21 19:01 overlookmotel

To be honest, it's very unclear. I personally think https imports are a horrifically bad idea, and i hope node never supports them, but the main thing is that until node's path is clear i need to keep resolve open to matching it.

ljharb avatar Jan 08 '21 19:01 ljharb

@ljharb Thanks for explaining. Makes sense now!

overlookmotel avatar Jan 09 '21 16:01 overlookmotel

That said... it seems like a pretty niche case. So if it were practical to provide a sync version (with an explicit warning that it does not support async resolution like https import), it could still be useful in the majority of cases.

I am using enhanced-resolve (Webpack's implementation) for now, which does provide sync resolution. But my understanding is that everyone will switch to resolve library once ESM support is added, and other implementations will likely fall into disrepair. So just wanted to flag sync resolution is useful for at least some users.

I'll stop banging on about this now!

overlookmotel avatar Jan 09 '21 16:01 overlookmotel

This is a blocker for Vite, see https://github.com/vitejs/vite/discussions/4921#discussioncomment-1376930

brillout avatar Oct 01 '21 06:10 brillout

regarding the package exports - they're no longer exclusive to ESM. the documentation is moved to Modules: Packages and applies to CJS too

considering the number of packages that rely on resolve (jest, eslint-plugin-import, etc.) and the number of packages that are starting to drop the main field in favor of exports, should this feature be implemented separately to ESM support?

because right now require.resolve(X) and resolve(X) behaves differently for CJS

dreyks avatar Dec 02 '21 22:12 dreyks

@dreyks they've never been exclusive to ESM :-)

Yes, absolutely. The plan has always been to add exports support for CJS first, and then for ESM separately, but 99% of the work is in that first part.

ljharb avatar Dec 02 '21 23:12 ljharb

oh, i see. i got confused by the issue title and by the fact that the initial doc link leads to "ESM: Packages"

dreyks avatar Dec 03 '21 07:12 dreyks

As more and more packages take the (IMO user-hostile) approach of dropping the "main" field in package.json, it seems that exports support in this package is one of the blockers to tools like ESLint and others. I see a stale PR (https://github.com/browserify/resolve/pull/224) with some work to support exports, with no activity for over a year, and no activity here in this issue.

Is support for exports being worked on currently? Is it blocked by other packages? Have many tools moved on to alternatives like enhanced-resolve? It sounds like eslint-import still is waiting for support in this package, rather than changing to something else.

I'm not trying to apply any pressure, but if the maintainers were able to provide an update on the current situation and any kind of guidance on the next steps to be taken and whether there is a plan to take them, I'd appreciate it.

IanVS avatar Dec 13 '22 16:12 IanVS

@IanVS yes, it's something i very much want to complete. I'll try to prioritize it this month.

ljharb avatar Dec 13 '22 17:12 ljharb

What if I just write a new resolver? I have a lot of experience now with generator-based architectures. Generators are the most modern/decent way to write code once and have it work in both sync and async contexts, and sync generators have good environment support these days. I think I can create something that will be quite a bit more friendly to use than resolve or enhanced-resolve just by applying better (newer) patterns.

conartist6 avatar Jan 21 '23 16:01 conartist6

@ljharb Would you be interested in collaborating on something like that?

conartist6 avatar Jan 21 '23 16:01 conartist6

I am highly disinclined to use generators; i find them horrific to maintain. I’d prefer to adapt the current code to support “exports” rather than try a rewrite (something that is virtually always a disaster).

Additionally, using generators or iterators would mean we couldn’t support down to node 0.4, which is absolutely critical for this package.

ljharb avatar Jan 21 '23 20:01 ljharb

OK, that makes sense re 0.4. I still think there's value in creating a radically simpler more modern package though, even if there's a need to maintain something that has deep-rooted legacy limitations.

I have found that generators can be used to simplify code like this greatly, particularly when you use them as strategies that control the transitions of a state machine. If you design the state machine API correctly it essentially becomes a de-facto plugin API because you can always use higher-order strategies to extend any existing functionality.

An immediate gain is that you end up with normal top-down control, and thus useful stack traces! And of course you can provide sync and async variants with a single implementation. And it becomes possible to create a visual state machine, devtools for resolvers essentially. This isn't a big deal when you have one resolver, but what we actually need is a huge family of related resolvers, so that it rapidly becomes useful to be able to explain at any given moment what a resolver is doing and WHY.

conartist6 avatar Jan 21 '23 21:01 conartist6

@ljharb As @IanVS stated I don't want to add to the pressure and I know you are a busy guy doing a lot of things for free. As you've stated so many other things would be fixed (like eslint-plugin-import) if resolve supports ESM resolution. Given the many problems with build tooling around the big push towards ESM (especially when it comes to consuming ESM-only packages) it would be really great to finally fix this issue. Just trying to be a bit of a squeaky wheel without being too annoying...

dlong500 avatar Feb 07 '23 06:02 dlong500