eslint-plugin-jsdoc icon indicating copy to clipboard operation
eslint-plugin-jsdoc copied to clipboard

TypeScript error when require eslint-plugin-jsdoc in a CommonJS file

Open shadowspawn opened this issue 10 months ago • 6 comments

Expected behavior

Able to use TypeScript for type checking in a CommonJS file which imports eslint-plugin-jdocs (using require).

Actual behavior

TypeScript fails on the require line for eslint-plugin-jdocs in a CommonJS file.

$ tsc -p tsconfig.js.json

eslint.config.js:6:23 - error TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("eslint-plugin-jsdoc")' call instead.
  To convert this file to an ECMAScript module, change its file extension to '.mjs' or create a local package.json file with `{ "type": "module" }`.

6 const jsdoc = require('eslint-plugin-jsdoc');

My understanding of the problem is that TypeScript best practice is a definition file corresponds to a single JavaScript file. I am guessing when TypeScript finds the types file for eslint-plugin-jdocs of ./dist/index.d.ts, it assumes the corresponding JavaScript file will be the esm flavour since eslint-plugin-jdocs has "type": "module" and the .d.ts is therefore implicitly esm.

There is a project which champions technically correct TypeScript setups and picks up a potential problem. This might be more convincing than my scenario. 😄

  • https://arethetypeswrong.github.io/?p=eslint-plugin-jsdoc%4048.2.3
  • https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseESM.md

Quoting from FalseESM:

A golden rule of declaration files is that if they represent a module—that is, if they use import or export at the top level—they must represent exactly one JavaScript file. They especially cannot represent JavaScript files of two different module formats.

I was able to fix my error without actually creating a separate definition file by modifying eslint-plugin-jdocs package.json file like this, but I suspect it is not robust so more for interest than a suggestion!

  "exports": {
    "import": {
      "types": "./dist/index.d.ts",
      "default": "./src/index.js"
    },
    "require": {
      "types": "./dist/index.d.ts",
      "default": "./dist/index.cjs"
    }
  },

I assume the robust solution would be to have an explicit definition file for cjs (say): index.d.cts.

TypeScript is normally fairly forgiving about this problem to avoid breaking consumers (https://github.com/microsoft/TypeScript/issues/50762#issuecomment-1528318260) and I seem to have stumbled on a case where it notices.

Happy to work on a simple reproduction if you would like to see TypeScript failure yourself rather than just the arethetypeswrong warning, and willing to work on a PR.

Environment

  • Node version: v20.11.1
  • ESLint version v8.56.0
  • eslint-plugin-jsdoc version: 48.2.3
  • TypeScript version: 5.2.2

shadowspawn avatar Apr 05 '24 06:04 shadowspawn

I checked the TypeScript moduleResolution in my project. Ah ha, my tsconfig has node16 and the arethetypeswrong warning is for node16+CommonJS, that lines up.

  "compilerOptions": {
    "module": "node16",

shadowspawn avatar Apr 13 '24 04:04 shadowspawn

Hmm, this is potentially tricky. TypeScript is not the tool for generating multiple versions of definition files. They recommend you generate one of ESM/CJS from your TypeScript and then use a tool to transpile the files for the other, like Babel (as used here). But that process includes generating both the JavaScript files and the type definition files. And Babel only generates the JavaScript and not the definition file. More investigation required...

shadowspawn avatar Apr 15 '24 06:04 shadowspawn

I'm working on fixing this, I should have a PR ready soon.

aryaemami59 avatar Aug 24 '24 21:08 aryaemami59

Just out of curiosity, would you guys be open to a full-on TypeScript migration? I can fix this issue and handle the migration (it wouldn't be that difficult since we already have the types in JSDoc). If you give me the thumbs up, I can go ahead and get started on it.

aryaemami59 avatar Aug 24 '24 21:08 aryaemami59

It's up to @gajus , but I'd personally hope for it to remain as TS-flavored JavaScript.

brettz9 avatar Aug 24 '24 21:08 brettz9

@brettz9 We might be able to keep the TS-flavored JavaScript, but it's so much easier to make the switch. Here is a before and after:

Before:

broken-type-defs-eslint-plugin-jsdoc

After:

good-type-defs-eslint-plugin-jsdoc

aryaemami59 avatar Aug 24 '24 21:08 aryaemami59