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

Bug: no-missing-import doesn't use the require algorithm when TS has CJS moduleResolution? #349

Closed
1 task
hakimio opened this issue Oct 3, 2024 · 31 comments
Labels
bug rule:update An update to a current rule typescript

Comments

@hakimio
Copy link

hakimio commented Oct 3, 2024

Environment

Node version: v20.13.1
npm version: v10.8.1
ESLint version: v9.11.1
eslint-plugin-n version: v17.10.3
Operating System: Windows 10

What rule do you want to report?

no-missing-import

What did you expect to happen?

The plugin should find imports from TS barrel files.

Participation

  • I am willing to submit a pull request for this issue.

Additional comments

No response

@hakimio hakimio added the bug label Oct 3, 2024
@voxpelli
Copy link
Member

voxpelli commented Oct 3, 2024

Can you elaborate a bit on the "TS barrel files" and maybe point to some example or docs? There's barely any info in this issue, making it harder than it needs to be to act upon 🙏

@hakimio
Copy link
Author

hakimio commented Oct 3, 2024

@voxpelli
Copy link
Member

voxpelli commented Oct 3, 2024

Right, that concept I’m aware of, not sure that’s the common name for it though, never really heard that term before.

Is this limited to TS or goes for all files that does that kind of thing?

And what error are you getting?

From the rule description:

This rule checks the file paths of import and export declarations

But “should find imports from TS barrel files” sounds like it’s not about paths in your case but about what’s imported? Path validation shouldn’t care if it’s a “barrel file” or non-barrel one

@hakimio
Copy link
Author

hakimio commented Oct 3, 2024

Following throws errors for me:

import {Foo} from '../../foo/bar';

where bar is existing folder containing index.ts exports.

@voxpelli
Copy link
Member

voxpelli commented Oct 3, 2024

Right, then the issue is probably this:

You use TS-syntax that compiles to CommonJS syntax, since only CommonJS (require()) supports resolving ./something to ./something/index.js

ESM imports are required to include the full path, including file extensions.

Have you specified your tsconfig? https://github.com/eslint-community/eslint-plugin-n/blob/master/docs/rules/no-missing-import.md#tsconfigpath

@hakimio
Copy link
Author

hakimio commented Oct 3, 2024

No, I am not using CommonJS anywhere in the application. I am using ESM everywhere and everything transpiles just fine.
ts-node for development and esbuild for production builds.
I am not using full paths (or extensions) anywhere and everything works.

@hakimio
Copy link
Author

hakimio commented Oct 3, 2024

image

@scagood
Copy link

scagood commented Oct 3, 2024

So it's exactly what @voxpelli said:

You use TS-syntax that compiles to CommonJS syntax, since only CommonJS (require()) supports resolving ./something to ./something/index.js

ESM imports are required to include the full path, including file extensions.

You will need to specify the full path to the file path (including file extension)
Docs: Mandatory file extensions

So, this will error:
import { cat } from './interfaces';

And this will not:
const { cat } = require('./interfaces');


Edit:

There is also no way to change this currently:
https://github.com/eslint-community/eslint-plugin-n/blob/master/lib/util/import-target.js#L279-L286

@scagood scagood added enhancement typescript rule:update An update to a current rule and removed bug labels Oct 3, 2024
@hakimio
Copy link
Author

hakimio commented Oct 3, 2024

Ok, if TS code with barrel imports is not supported and there are no plans to introduce the support, feel free to close this as "won't fix".

@scagood
Copy link

scagood commented Oct 3, 2024

It should be doable, the basics is just adding mainFiles.push("index") to https://github.com/eslint-community/eslint-plugin-n/blob/master/lib/util/import-target.js#L293. However, we would need to ensure we don't break other ts files (eg if ts gets compiled to esm).

The other option would be an option called allowIndexImports that is user configurable.

A final option would be to expose the resolver options found here:
https://github.com/eslint-community/eslint-plugin-n/blob/master/lib/util/import-target.js#L296, but that is a little trickier


This is also useful if --experimental-specifier-resolution is set in node js

@hakimio
Copy link
Author

hakimio commented Oct 3, 2024

allowIndexImports sounds like a good workaround to me.

@voxpelli
Copy link
Member

voxpelli commented Oct 3, 2024

The functionality is called Using folders as modules in the CJS docs of Node.js, and is considered legacy with subpath imports being the recommended replacement.

The requirement to specify index.js in ESM-files is also documented here: https://nodejs.org/api/esm.html#mandatory-file-extensions

I think rather than allowIndexImports it should be useRequireResolution or such, as the algorithm that require() supports more special cases than index files.

@hakimio
Copy link
Author

hakimio commented Oct 3, 2024

ts-node supports ESM barrel imports without any issue out-of-the-box without any special flags. It's only an issue with vanilla nodejs ESM imports.
I do not think full require() algorithm should be supported and useRequireResolution is a strange name for the config - nobody will understand what it does.

@voxpelli
Copy link
Member

voxpelli commented Oct 3, 2024

ts-node supports ESM barrel imports without any issue out-of-the-box without any special flags.

Have you investigated why / how it does that?

Does it still support it if you set these in your tsconfig.json?

    "module": "nodenext",
    "moduleResolution": "nodenext",

@hakimio
Copy link
Author

hakimio commented Oct 3, 2024

Yes, it works fine with module resolution set to NodeNext. Here is my tsconfig:

{
    "compilerOptions": {
        /* Base Options: */
        "esModuleInterop": true,
        "skipLibCheck": true,
        "allowJs": true,
        "resolveJsonModule": true,
        "isolatedModules": true,
        "strictNullChecks": true,
        "strictPropertyInitialization": false,
        "forceConsistentCasingInFileNames": true,
        "allowSyntheticDefaultImports": true,
        /* Strictness */
        "strict": true,
        /* Decorators */
        "experimentalDecorators": true,
        "emitDecoratorMetadata": true,
        "target": "ES2021",
        "module": "NodeNext",
        "moduleResolution": "NodeNext",
        "outDir": "dist",
        "types": [
            "./typings.d.ts"
        ]
    },
    "include": [
        "./srv"
    ]
}

@voxpelli
Copy link
Member

voxpelli commented Oct 3, 2024

According to ts-node, they require that you set both "type": "module" in package.json and "module": "ESNext" in tsconfig.json to use ESM output, and then it uses the new experimental ESM loaders and otherwise they use the old require.extensions that has been deprecated since Node.js v0.10.6 and carries the following note:

Avoid using require.extensions. Use could cause subtle bugs and resolving the extensions gets slower with each registered extension.


Relatedly: I did an investigation related to this now, as they have done some work on enabling ESM-files to be loaded through require() , and ran into some odd findings, but that should not be relevant here: https://github.com/voxpelli/investigation-esm-require

I did still ask the author of that feature about it on Twitter: https://twitter.com/voxpelli/status/1841818608713826693

@hakimio
Copy link
Author

hakimio commented Oct 3, 2024

My code is ESM TS but for production I am transpiling to CJS. So, the output is CJS.
Anyway, why does it matter for this rule how the transpiled code looks like? I am not running the linter on the minified production bundle.

@voxpelli
Copy link
Member

voxpelli commented Oct 3, 2024

why does it matter for this rule how the transpiled code looks like

It matters because different module resolution is applied when a javascript file is loaded through require and when its loaded through import and TS annoyingly supports both in their import so its important to know if its a bug in the implementation of either of the module resolutions or a matter of the wrong module resolution being applied.

Sounds to me like no-missing-import is applying the wrong module resolution in your case.

If you switch to "module": "commonjs" in your tsconfig.json and sets "type": "commonjs" in your package.json – does the issue still exist?

@hakimio
Copy link
Author

hakimio commented Oct 4, 2024

If you switch to "module": "commonjs" in your tsconfig.json and sets "type": "commonjs" in your package.json – does the issue still exist?

Yes, I get exactly the same errors.

TS annoyingly supports both in their import

Sorry, but I have to strongly disagree. The annoying one here is nodejs which for a decade or so can't implement proper support for ES modules. Now we have to resort to stupid workarounds like .mjs/.mts/.cjs/.cts or "type": "module" and nodejs community is divided into CJS and ESM camps. CJS should have been deprecated years ago and everyone should have moved to ESM. Why runtimes like bun can support both CJS and ESM module systems in a single file and nodejs plain refuses to implement this? Just look at ESLint v9 - do you know how many different config extensions it has now? 6 different variants. That's just plain ridiculous.

@voxpelli
Copy link
Member

voxpelli commented Oct 4, 2024

Yes, I get exactly the same errors.

Then that could be a bug. @scagood When a TS-file is configured to have CJS module resolution, does no-missing-import use the require resolution algorithm rather than the import one?

Why runtimes like bun can support both CJS and ESM module systems in a single file and nodejs plain refuses to implement this?

Node.js supports importing CJS files in ESM files by using import and will be able to import most ESM files in CJS files using require when Node 23 is launched (already possible in latest Node 22 when using --experimental-require-module) and CJS files can already import ESM files by using await import()

The issue here though isn't whether require / import supports both ESM and CJS syntax or whether both ESM and CJS files implement them both.

The issue is that TypeScript in some cases apply the require algorithm to the import statements, which is straight up incorrect, and in your case no-missing-import isn't adapting to that and giving you errors.

Only thing that remains to be figured out is why no-missing-import isn't adapting.

@voxpelli voxpelli changed the title Bug: no-missing-import doesn't work with TS barrel files (index.ts) Bug: no-missing-import doesn't use the require algorithm when TS has CJS moduleResolution? Oct 4, 2024
@voxpelli voxpelli added bug and removed enhancement labels Oct 4, 2024
@hakimio
Copy link
Author

hakimio commented Oct 4, 2024

Node.js supports importing CJS files in ESM files by using import and will be able to import most ESM files in CJS files using require when Node 23 is launched

But you still can't use require() and import() in the same file like with bun? That's the issue. There shouldn't be any divide and there shouldn't be any need for cjs and mjs file formats.

CommonJS is hurting JavaScript

@voxpelli
Copy link
Member

voxpelli commented Oct 4, 2024

But you still can't use require() and import() in the same file like with bun?

You can use await import() in CJS files and you can create a require() in ESM files using createRequire():

import { createRequire } from 'node:module';
const require = createRequire(import.meta.url);

// sibling-module.js is a CommonJS module.
const siblingModule = require('./sibling-module'); 

There shouldn't be any divide and there shouldn't be any need for cjs and mjs file formats.

Those currently are only needed for the type that differs from the "type" of the package.json.

As mentioned, starting in Node 23 – and already by using --experimental-require-module in latest Node 22 – it will no longer be needed in many cases (whenever no top level await exists in the dependency chain) as Node.js will then try requiring a file as CJS and if that fails it will retry it as ESM. (It can still be advisable to use "type" or .mjs for ESM-files when using require though, as the performance will be a bit better, as it won't have to try requiring it as CJS first)


That said, lets focus on supporting the require algorithm in import for such TS projects, as looking at the tests and code it seems like there are no logic for that case at all at the moment

@voxpelli
Copy link
Member

voxpelli commented Oct 4, 2024

@hakimio You could try import/no-unresolved / import-x/no-unresolved as they are the more advanced version of n/no-missing-import / n/no-missing-require and since eslint-plugin-import / eslint-plugin-import-x focuses on nothing but this very functionality it should have wider support for the cases, including full support for TypeScript

@hakimio
Copy link
Author

hakimio commented Oct 4, 2024

What's the difference between those two?

@scagood
Copy link

scagood commented Oct 4, 2024

Then that could be a bug. @scagood When a TS-file is configured to have CJS module resolution, does no-missing-import use the require resolution algorithm rather than the import one?

Currently, the rule does not distinguish between common and module resolution, it just does esm resolution on no-missing-import and node resolution on no-missing-require 👀
I have been thinking about merging these two together into just no-missing, maybe we could perform the package.json#type check then


What's the difference between those two?

eslint-plugin-import-x is a fork of eslint-plugin-import that aims to provide a more performant and more lightweight version of the original plugin.

@voxpelli
Copy link
Member

voxpelli commented Oct 4, 2024

Currently, the rule does not distinguish between common and module resolution, it just does esm resolution on no-missing-import and node resolution on no-missing-require 👀 I have been thinking about merging these two together into just no-missing, maybe we could perform the package.json#type check then

That would be nice, for this specific case its more a matter of checking moduleResolution than package.json#type though. We could check what https://github.com/import-js/eslint-import-resolver-typescript is doing and copy the necessary parts if we want to entertain that path – or we simply say that TS-specific resolution algorithms is out of scope of eslint-plugin-n

@hakimio
Copy link
Author

hakimio commented Oct 4, 2024

or we simply say that TS-specific resolution algorithms is out of scope

Considering that there was effort put to improve TS support (#139) that would be a big step backwards. Especially when the fix for the issue is quite simple:

It should be doable, the basics is just adding mainFiles.push("index")

@voxpelli
Copy link
Member

voxpelli commented Oct 4, 2024

Especially when the fix for the issue is quite simple

That's an incorrect fix. The correct fix is to implement the correct algorithm informed by what TS considers to be the correct algorithm.

That involves a few things:

  1. Figure out what TS considers for the current project
  2. Interpret what algorithm that translates to
  3. Start using the require algorithm in the cases where its needed

@hakimio
Copy link
Author

hakimio commented Oct 4, 2024

That might be way beyond this rule's scope...

@voxpelli
Copy link
Member

voxpelli commented Oct 4, 2024

Exactly my feeling, especially as import/no-unresolved etc has a good pluggable file resolution system with proper TS support

@hakimio hakimio closed this as not planned Won't fix, can't repro, duplicate, stale Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug rule:update An update to a current rule typescript
Projects
None yet
Development

No branches or pull requests

3 participants