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

Rule import/no-unresolved raise false positive when there is no main field in the package.json #2132

Open
macabeus opened this issue Jun 19, 2021 · 40 comments

Comments

@macabeus
Copy link

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

import GBAEmulator, { GbaContext } from 'react-gbajs'

Full code

./node_modules/.bin/eslint .

What did you expect to happen?
Should raise no error. This package is present in the node_modules.

An important note:
Since it's only for the browser, it has the field browser instead of main in its package.json (link). I made it following the NPM documentation.
Despite that, I noticed that by adding the field main in its package.json, the error suppress - but it doesn't look correct.

What actually happened? Please copy-paste the actual, raw output from ESLint.

/Users/macabeus/ApenasMeu/klo-gba.js/brush/src/components/Emulator/index.js
  2:41  error  Unable to resolve path to module 'react-gbajs'  import/no-unresolved

/Users/macabeus/ApenasMeu/klo-gba.js/brush/src/components/KloGbaSidebar/Hacks/index.js
  7:28  error  Unable to resolve path to module 'react-gbajs'  import/no-unresolved

/Users/macabeus/ApenasMeu/klo-gba.js/brush/src/hooks/useGbaSaveRestoreState.js
  2:28  error  Unable to resolve path to module 'react-gbajs'  import/no-unresolved

/Users/macabeus/ApenasMeu/klo-gba.js/brush/src/index.js
  8:29  error  Unable to resolve path to module 'react-gbajs'  import/no-unresolved

/Users/macabeus/ApenasMeu/klo-gba.js/brush/src/providers/VisionProvider.js
  4:28  error  Unable to resolve path to module 'react-gbajs'  import/no-unresolved

✖ 5 problems (5 errors, 0 warnings)

✨  Done in 4.73s.

Steps to reproduce this issue:

  1. Clone this repository: https://github.com/macabeus/klo-gba.js
  2. Run yarn on its root
  3. cd brush
  4. yarn run lint
@ljharb
Copy link
Member

ljharb commented Jun 19, 2021

A package just for the browser still needs node behavior, even if that’s “nothing”, so having it lack a main is a bug.

That said, it seems reasonable that when there’s no “main” present, but there is a browser field, that we respect it for the purposes of this rule - although i worry that would hide a bug in the common case.

@macabeus
Copy link
Author

I think that it would be better not to demand the main field when browser is present.
NPM doc is reasonable when explaining that it's an "instead of". Otherwise, I'm losing the hint that it's only for the browser.

browser
If your module is meant to be used client-side the browser field should be used instead of the main field. This is helpful to hint users that it might rely on primitives that aren't available in Node.js modules. (e.g. window)

@ljharb
Copy link
Member

ljharb commented Jun 19, 2021

Perhaps "main": false then - a missing main might just mean it's pointing to a missing index.js, since that's the default.

@macabeus
Copy link
Author

macabeus commented Jun 19, 2021

Testing using false... And raises a warning on VSCode because the expected type is string.
Probably it'll raise an error on other tools too.
image

I also tested using an empty string, and the lint error continues.

@ljharb
Copy link
Member

ljharb commented Jun 20, 2021

false is a valid value; that's just the package.json typescript types being broken. If it's false, does the linter pass?

@macabeus
Copy link
Author

macabeus commented Jun 20, 2021

There are the same lint errors even when using false.

@gautaz
Copy link

gautaz commented Jul 6, 2021

The same error occurs for the p-queue package which package.json only shows an exports field and no main field.

I agree that this is rather odd considering what is currently advised on the NodeJS site.

christophehurpeau added a commit to christophehurpeau/check-package-dependencies that referenced this issue Jul 11, 2021
@LumaKernel
Copy link

I'm using monaco-editor (0.26.1) and getting no-unresolved error with import ... from 'monaco-editor'.

monaco-editor has following package.json:

...
  "typings": "./esm/vs/editor/editor.api.d.ts",
  "module": "./esm/vs/editor/editor.main.js",
...

https://github.com/microsoft/monaco-editor/blob/v0.26.1/package.json#L15-L16

@ljharb
Copy link
Member

ljharb commented Jul 20, 2021

@LumaKernel that package.json is incorrect. The module field is not a thing, and the lack of main/exports combined with private:true means nobody can, or should be able to, import/require from that package.

@benmccann
Copy link

benmccann commented Jul 20, 2021

I ran into this with periscopic 3.0.3, which has the following in package.json:

  "module": "src/index.js",
  "type": "module",
  "exports": {
    "import": "./src/index.js"
  },

Seems like this could also be fixed by support exports (#1868, #1810)

@ljharb
Copy link
Member

ljharb commented Jul 21, 2021

@benmccann yep! That ones a valid one, but since it lacks a main (which it should have for back compat support anyways), we can’t resolve it until exports is supported by resolve.

@niieani
Copy link

niieani commented Sep 13, 2021

This is also raised for packages that are typescript definitions-only.
One such example is callbag package, that only exports TypeScript types.
When I do:

import type { Sink, Source } from 'callbag'

Rule import/no-unresolved is raised. But in this case only types field should be resolved, since there is no runtime behavior here. Ideally, all import type could probably be ignored, since TypeScript itself will error when it can't be resolved.

@ljharb
Copy link
Member

ljharb commented Sep 13, 2021

@niieani import type should be completely ignored by no-unresolved, i think - does it warn on that on the latest version of the plugin?

@niieani
Copy link

niieani commented Sep 13, 2021

@ljharb yup, simplest reproduction is that line I cited above. Install callbag and try that import type with the rule turned on and you'll get an error from eslint.

@ljharb
Copy link
Member

ljharb commented Sep 13, 2021

I mean, that package is super broken because it has a "main" that doesn't exist - it should specify "main": false if it's not meant to be importable. You may want to file a bug on it.

That said, we should be using package.json, not the main, to verify import type packages, so i think that's the fix here.

@ghoullier
Copy link

All package authored by @sindresorhus will be migrated to pure esm module see article
It's already the case for chalk and many other packages.

@ljharb
Copy link
Member

ljharb commented Dec 28, 2021

@ghoullier yes, i'm aware - and that's very user-hostile and will break a ton of things, including this plugin.

I suggest switching to packages that preserve compatibility.

@kachkaev
Copy link

kachkaev commented Dec 29, 2021

I have just upgraded a side-project to ESM – it consists of data-processing scripts and a Next.js app for data visualisations. Everything seems to be working with "type": "module" in package.json, thanks to experimental ESM support in ts-node and to a small temp hack in next.config.mjs.

For me, this unlocked execa@v6 , chalk@v5 and sort-keys@v5. All of these project dependencies are authored by sindresorhus and are ESM-only.

Although I have no runtime issues, importing chalk unexpectedly triggers ESLint:

import chalk from "chalk"; // ❌ Unable to resolve path to module 'chalk'. eslint(import/no-unresolved)

The other two very similar packages don’t produce any ESLint errors. Here are package.json of the versions I am using:

Does anyone see anything that would break import/no-unresolved only for chalk? 🤔

In the meantime, I’ll stick with "import/no-unresolved": "off" and will expect yarn lint:tsc to monitor imports.

P.S.: Pure ESM packages are somewhat painful, but I appreciate the migration pressure created by maintainers. The faster we move the ecosystem to ESM-only, the better for everyone. This year I had to explain differences between ESM and CommonJS to new devs and also assist with quite intricate repo configs. I hope all this knowledge becomes unnecessary for most folks by the end of 2022!

@ljharb
Copy link
Member

ljharb commented Dec 29, 2021

The pressure you're discussing is not actually helping the ecosystem; i invite you to check the per-version download counts for every package that's gone ESM-only. It's a clear signal that developers do not want that.

The reason is because sindre is actually authoring their packages incorrectly - by omitting main entirely, there's a default "main" of index.js, which execa and sort-keys have. The proper way to have an ESM-only package would be "main": false, which would then correctly create a warning for all of them.

Regardless of this plugin's inevitable eventual support for ESM resolution - which should of course work - I would strongly encourage you to migrate to package authors that maintain compatibility over ones that attempt to subjugate their users by forcing them to use a specific subset of node's available module systems.

@c-vetter
Copy link

@ljharb
You claim false to be valid for main. Can you please cite your source?

The official docs for the field don't mention false:

On the other hand, the nodejs.org pages list type as <string>.
Plus, https://nodejs.org/docs/latest-v16.x/api/modules.html shows this pseudo code:

LOAD_AS_DIRECTORY(X)
1. If X/package.json is a file,
   a. Parse X/package.json, and look for "main" field.
   b. If "main" is a falsy value, GOTO 2.
   c. let M = X + (json main field)
   d. LOAD_AS_FILE(M)
   e. LOAD_INDEX(M)
   f. LOAD_INDEX(X) DEPRECATED
   g. THROW "not found"
2. LOAD_INDEX(X)

1.b indicates that false would not that the package cannot be required but that index.js would be tried.

@ljharb
Copy link
Member

ljharb commented Feb 11, 2022

@rasenplanscher mkdir -p node_modules/main-false && echo '{"main": false}' > node_modules/main-false/package.json && touch node_modules/main-false/index.js && node -pe "require.resolve('main-false')" does show that you're right. I'll add a test case to resolve for this.

That still makes "omit main entirely" an incorrect approach, unless there's no index.js present - which the packages mentioned above have.

In that case, my testing shows that setting main to {} will, at least, error in every node version down to 0.10 (0.6 and 0.8 still default to index.js in that case), and "exports" takes precedence over this incorrect "main" in node versions that support it. Probably a better alternative, then, is to have an explicit main that points to a file that exists, but throws a runtime error immediately upon being required.

@sindresorhus

This comment was marked as resolved.

@ljharb

This comment was marked as resolved.

@danielweck
Copy link

This workaround works for me: https://gist.github.com/danielweck/cd63af8e9a8b3492abacc312af9f28fd

@silverwind
Copy link
Contributor

silverwind commented Apr 18, 2023

Could exports just be treated as an alias to main for the purpose of linting?

Or maybe this module could just use either require.resolve which does not have the issue. Also there's experimental import.meta.resolve.

@ljharb
Copy link
Member

ljharb commented Apr 18, 2023

@silverwind that would be trivial when "exports" is a string, but the formats node supports for it are varied and complex.

I've tried import.meta.resolve and it's horribly broken and not useful.

@silverwind
Copy link
Contributor

silverwind commented Apr 18, 2023

What do you think about starting with support for string form exports and an alias to main? It would probably cover a big percentage of packages. Object form could be supported later. I could likely provide a PR for that.

@ljharb
Copy link
Member

ljharb commented Apr 18, 2023

That's certainly a viable option, although I'm still uncertain about it. Most packages should be using the object form so they can expose package.json also, otherwise their package won't work with a bunch of tooling.

Notably the package from the OP doesn't have exports - https://unpkg.com/browse/react-gbajs@1.0.2/package.json and another mentioned above ( https://github.com/Rich-Harris/periscopic/blob/58d8184ddd3808db26340b6e8c7f6b3ef8811d9f/package.json#L8-L10 ) has an object form using import.

@silverwind
Copy link
Contributor

Most packages should be using the object form so they can expose package.json also, otherwise their package won't work with a bunch of tooling.

Never heard of this. How would such a package.json export look? When a package declares a string exports, it should still be possible to import any file like package.json via specifier module/package.json, so I don't see a pressing need for this.

@ljharb
Copy link
Member

ljharb commented Apr 18, 2023

"./package.json": "./package.json".

and no, with a string exports nothing is exposed except the package name itself, that's the whole point.

@silverwind
Copy link
Contributor

silverwind commented Apr 18, 2023

You're right, if I try that for such a module, I get

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './package.json' is not defined by "exports"

I guess it could still be worked around via manual resolve to absolute path and file:// import.

@ljharb
Copy link
Member

ljharb commented Apr 18, 2023

Not reliably - because if the main is in a subfolder, that might have a package.json itself, it gets tricky. That's a big part of why it's a best practice for packages using exports to always explicitly expose package.json.

@so1ve
Copy link

so1ve commented Apr 23, 2023

Actually I point main to an ESM file in my projects. It still works well and will throw when the package is imported by require. I do think package authors need to add this field back, but eslint-plugin-import should reconsider add support for exports.

Anyway, thank you for your hard work!

@silverwind
Copy link
Contributor

silverwind commented Apr 23, 2023

Yes, I do that as well. I just add a dummy main pointing to a ESM file to make this plugin work. CJS import will be broken anyways for ESM packages, so it does not matter whether main points to a valid CJS file.

@akx
Copy link

akx commented Jan 22, 2024

For p-queue, I'm working around this with a custom resolver:

function resolve(source) {
  if (source === 'p-queue') {
    return {found: true, path: require.resolve("p-queue")};
  }
  return {found: false};
}

exports.interfaceVersion = 2;
exports.resolve = resolve;

Funny that require.resolve() automatically does the right thing (Node 21.5.0) – what would be the downsides, if any, to the built-in Node resolver trying that first?

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

No branches or pull requests