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 ESM resolution #222

Open
SimenB opened this issue Apr 25, 2020 · 37 comments
Open

Support ESM resolution #222

SimenB opened this issue Apr 25, 2020 · 37 comments

Comments

@SimenB
Copy link
Contributor

SimenB commented Apr 25, 2020

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
  • 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.
    • Support a promise API #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: nodejs/node#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.

@ctavan
Copy link

ctavan commented May 18, 2020

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 react-native-community/cli#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: nodejs/node#33460

@ljharb
Copy link
Member

ljharb commented May 18, 2020

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.

@SimenB
Copy link
Contributor Author

SimenB commented May 19, 2020

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 😀

@ctavan
Copy link

ctavan commented May 19, 2020

@SimenB thanks for this suggestion! I took the liberty to copy it over to nodejs/node#33460 (comment) 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 😉 .

@ljharb
Copy link
Member

ljharb commented May 19, 2020

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

@SimenB
Copy link
Contributor Author

SimenB commented May 19, 2020

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

@ljharb
Copy link
Member

ljharb commented May 19, 2020

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

@NMinhNguyen
Copy link

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?

@SimenB
Copy link
Contributor Author

SimenB commented Jun 5, 2020

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

@franciscop
Copy link

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:

@ljharb
Copy link
Member

ljharb commented Jun 12, 2020

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

@overlookmotel
Copy link

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.

@ljharb
Copy link
Member

ljharb commented Jan 7, 2021

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.

@overlookmotel
Copy link

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.

@ljharb
Copy link
Member

ljharb commented Jan 7, 2021

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.

@overlookmotel
Copy link

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

@ljharb
Copy link
Member

ljharb commented Jan 8, 2021

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.

@overlookmotel
Copy link

overlookmotel commented Jan 9, 2021

@ljharb Thanks for explaining. Makes sense now!

@overlookmotel
Copy link

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!

@IanVS
Copy link

IanVS commented Dec 13, 2022

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 (#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.

@ljharb
Copy link
Member

ljharb commented Dec 13, 2022

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

@conartist6
Copy link

conartist6 commented Jan 21, 2023

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
Copy link

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

@ljharb
Copy link
Member

ljharb commented Jan 21, 2023

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.

@conartist6
Copy link

conartist6 commented Jan 21, 2023

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.

@dlong500
Copy link

dlong500 commented Feb 7, 2023

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

@dword-design
Copy link

Would be great to have this implemented since it blocks eslint-import-resolver-node from resolving packages that do not configure the main field 😀.

vimalloc added a commit to vimalloc/vite_ruby that referenced this issue Apr 17, 2023
Using eslint, I'm getting an `import/no-unresolved: Unable to resolve path to module 'vite-plugin-ruby'` when it clearly exists. Looking at [this ticket](browserify/resolve#222) I believe that is happening because there is no `main` key in the package.json. This updates the package.json to include this.
@benmccann
Copy link

Jest uses resolve as the default implementation in jest-resolve, although the user can swap it out

@SimenB I wonder if the most expedient solution would be to make resolve.exports or enhanced-resolve the default?

@SimenB
Copy link
Contributor Author

SimenB commented Jun 15, 2023

Jest uses resolve.exports to deal with exports and imports already

@LinusBF

This comment was marked as off-topic.

@conartist6

This comment was marked as abuse.

@conartist6

This comment was marked as resolved.

@ljharb

This comment was marked as resolved.

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

No branches or pull requests