search

lucaong / minisearch

Tiny and powerful JavaScript full-text search engine for browser and Node
https://lucaong.github.io/minisearch/
MIT License
4.64k stars 133 forks source link

Typescript with module resolution "NodeNext" + type: "commonjs" fails to import the commonJS module #255

Closed ItamarGronich closed 3 months ago

ItamarGronich commented 5 months ago

Description

When using with packageJson.type = 'commonjs' (the default) and typescript with the following config:

{
  "compilerOptions": {
    "module": "NodeNext",
    "moduleResolution": "NodeNext",
    "esModuleInterop": true
  }
}

tsc fails to load the commonjs module. After some research apparently tsc loads the ./dist/types/index.d.ts file from the packageJson.exports.type which makes typescript thing that the imported module is a esmodule and not a commonjs module.

That's because that in the index.d.ts file the way the code is exported is in esm format

export { type AsPlainObject, type AutoVacuumOptions, type BM25Params, type MatchInfo, type Options, type Query, type QueryCombination, type SearchOptions, type SearchResult, type Suggestion, type VacuumConditions, type VacuumOptions, type Wildcard, MiniSearch as default };

Expected behavior

tsc gets types that are applicable for both esm and commonjs.

Reproduction:

  1. download and unzip minisearch-types.tar.gz
  2. npm ci
  3. npm start
ItamarGronich commented 5 months ago

I've implemented a PR in #256. Let me know if there's anything you'd like me to change.

lucaong commented 4 months ago

Hi @ItamarGronich , thanks a lot for opening this issue and for sending a pull request. Allow me the time to reproduce the bug and fully understand the implications (I will be quite busy in the next few days, and I admit that tsconfig and module resolution in JS/TS sometimes evolves faster than I can follow), and I will review and hopefully merge it.

lucaong commented 4 months ago

@ItamarGronich I followed the steps for reproduction (thanks a lot for providing them, very helpful 👍 ). I indeed get an error when running npm run startrepor:

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("minisearch")' call instead.
  To convert this file to an ECMAScript module, change its file extension to '.mts', or add the field `"type": "module"` to '/Users/lucaongaro/Downloads/minisearch-types/package.json'.

1 import Minisearch from "minisearch";
                         ~~~~~~~~~~~~

Found 1 error in src/index.ts:1

This error, though, seems more related to the reproduction file src/index.ts and its package.json, rather than to MiniSearch types. Indeed, if I follow the recommendation in the error, and either change the extension to src/index.mts, or set "type": "module" in the package.json, the issue is solved without any change to MiniSearch, and npm run startrepor runs without errors.

Is that the same error you are getting?

ItamarGronich commented 4 months ago

Hi @lucaong, yes that's the same error.

However, I think this should work in all casses:

  1. "type": "commonjs" (default if type not supplied)
  2. "type": "module"
  3. index.mts
  4. index.cts
  5. index.ts (will use the value of "type" from package.json)

So this error shouldn't happen as tsc should know to select the cjs version of the code via the exports field in package.json.

So I think this error means that for some reason tsc fails to acknowledge that there's also a cjs version to load.

I think something is wrong with the way types are defined in package.json or maybe even a bug in tsc.

The expected behavior (the way i understand it) is like so:

1. Source code index.ts

import Minisearch from "minisearch";

// ... Do stuff with minisearch

2. Run tsc and it transpiles to cjs:

I want it to compile to cjs since my ecosystem runs on cjs yet.

const Minisearch = require('minisearch')

// ... Do stuff with minisearch

3. tsc should take cjs version

Now tsc should look at the package.json -> "exports" -> "." -> "require" -> "./dist/cjs/index.cjs" variant. And work as expected.

Note:

This should also work when you use *.mjs or "type": "module" since then the flow is: source -> tsc transpiles to esm -> package.json -> "exports" -> "." -> "require" -> "./dist/es/index.js" variant.

ItamarGronich commented 4 months ago

Hi @ItamarGronich , thanks a lot for opening this issue and for sending a pull request. Allow me the time to reproduce the bug and fully understand the implications (I will be quite busy in the next few days, and I admit that tsconfig and module resolution in JS/TS sometimes evolves faster than I can follow), and I will review and hopefully merge it.

Sadly, also i think that my PR doesn't solve the problem - although i thought it did. So hold off of the merge for now.

lucaong commented 4 months ago

@ItamarGronich I believe the issue is that when "type": "commonjs" in package.json, TypeScript expects all source files with extension .ts in the application to be CommonJS (and ES module files to use the .mts extension instead). This mirrors the same behavior from Node (expecting .js files to be CommonJS, and requiring .mjs for ES modules).

This only affects the application files (the src/index.ts file in the reproduction example), not the library files, which have their own package.json. The error is triggered when tsc encounters an unexpected import inside src/index.ts. Therefore, I believe it's not an issue with MiniSearch, and should instead be solved in the application code by applying one of the following alternatives:

At least this is my understanding of the issue, but feel free to comment further if you think there is actually an issue with MiniSearch.

ItamarGronich commented 4 months ago

I think that with tsc you can always use import / export, regardless of "type" field in the package.json.

What's changed with the "type" field is what format tsc compiles your code to.

lucaong commented 4 months ago

Uhm you are right, this is more complex than I hoped...

Relevant documentation:

ItamarGronich commented 4 months ago

Yeah it's more complicated.

Actually, i was doing a bit of research and i found this really cute package: https://www.npmjs.com/package/@arethetypeswrong/cli

I was skeptical of how it would work at first but wow it's awesome.

itamar.gronich minisearch $ attw --pack .

minisearch v6.3.0

Build tools:
- typescript@^5.2.2
- rollup@^4.1.0
- @rollup/plugin-typescript@^11.0.0

❗️ The resolved types use export default where the JavaScript file appears to use module.exports =. This will cause TypeScript under the node16 module mode to think an extra .default property access is required, but that will likely fail at runtime. These types should use export = instead of export default. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseExportDefault.md

👺 Import resolved to an ESM type declaration file, but a CommonJS JavaScript file. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseESM.md

💀 Import failed to resolve to type declarations or JavaScript files. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/NoResolution.md

┌───────────────────┬──────────────────────────────┬────────────────────────────┐
│                   │ "minisearch"                 │ "minisearch/SearchableMap" │
├───────────────────┼──────────────────────────────┼────────────────────────────┤
│ node10            │ ❗️ Incorrect default export  │ 💀 Resolution failed       │
├───────────────────┼──────────────────────────────┼────────────────────────────┤
│ node16 (from CJS) │ 👺 Masquerading as ESM       │ 👺 Masquerading as ESM     │
├───────────────────┼──────────────────────────────┼────────────────────────────┤
│ node16 (from ESM) │ 🟢 (ESM)                     │ 🟢 (ESM)                   │
├───────────────────┼──────────────────────────────┼────────────────────────────┤
│ bundler           │ 🟢                           │ 🟢                         │
└───────────────────┴──────────────────────────────┴────────────────────────────┘

So this read was actually really helpful and i think i know how to solve this now.

lucaong commented 4 months ago

This seems to be a solution: https://github.com/lucaong/minisearch/pull/258

It seems that the actual content of the declaration file does not matter, but its extension does: a .d.ts file assumes the presence of a .js file, and same for .d.mts (assumes .d.mjs) and .d.cts (assumes .d.cjs). Before, the type declaration was .d.ts, which matched the ES file dist/es/index.js but not the CJS file dist/cjs/index.cjs. Therefore, I now generate separate declaration files for CommonJS and ES, with identical content, but different extensions, and properly list them in the exports section of the package.json.

Could you please try it out too, and possibly test out with the package you found? To test it out, build it with yarn build or npm run build, then copy the package.json and dist folder into the node_modules/minisearch folder in the reproduction example.

ItamarGronich commented 4 months ago

Well the package doesn't like it.

itamar.gronich@ItamarGronich minisearch % attw --pack .

minisearch v6.3.1

Build tools:
- typescript@^5.2.2
- rollup@^4.1.0
- @rollup/plugin-typescript@^11.0.0

❌ Import resolved to JavaScript files, but no type declarations were found. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/UntypedResolution.md

❗️ The resolved types use export default where the JavaScript file appears to use module.exports =. This will cause TypeScript under the node16 module mode to think an extra .default property access is required, but that will likely fail at runtime. These types should use export = instead of export default. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseExportDefault.md

💀 Import failed to resolve to type declarations or JavaScript files. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/NoResolution.md

┌───────────────────┬──────────────────────────────┬──────────────────────────────┐
│                   │ "minisearch"                 │ "minisearch/SearchableMap"   │
├───────────────────┼──────────────────────────────┼──────────────────────────────┤
│ node10            │ ❌ No types                  │ 💀 Resolution failed         │
├───────────────────┼──────────────────────────────┼──────────────────────────────┤
│ node16 (from CJS) │ ❗️ Incorrect default export │ ❗️ Incorrect default export │
├───────────────────┼──────────────────────────────┼──────────────────────────────┤
│ node16 (from ESM) │ 🟢 (ESM)                     │ 🟢 (ESM)                     │
├───────────────────┼──────────────────────────────┼──────────────────────────────┤
│ bundler           │ 🟢                           │ 🟢                           │
└───────────────────┴──────────────────────────────┴──────────────────────────────┘

the reason is that export { ......... MiniSearch as default }; at the index.d.cts file.

This is because it's generated by a third party dts tool and not tsc.

lucaong commented 4 months ago

Unfortunately, I cannot seem to get the @rollup/plugin-typescript to generate a single .d.ts declaration file: it creates separate files for each source file instead. That's why I use rollup-plugin-dts, which on the other hand does not seem to generate the correct export syntax depending on the module type.

I wonder if Rollup is even needed for this. Maybe it could be used only for the UMD bundle, while the ES and CJS versions could be created with tsc alone.

ItamarGronich commented 4 months ago

Unfortunately, I cannot seem to get the @rollup/plugin-typescript to generate a single .d.ts declaration file: it creates separate files for each source file instead. That's why I use rollup-plugin-dts, which on the other hand does not seem to generate the correct export syntax depending on the module type.

Why do you need a single dts file? why can't it be many? Alothough, i think if you maybe use outFile instead of outDir you can maybe create a signle dts file? not sure. i know you can do that with tsc so maybe it can apply for @rollup/plugin-typescript.

I wonder if Rollup is even needed for this. Maybe it could be used only for the UMD bundle, while the ES and CJS versions could be created with tsc alone.

Yeah thought about it too. You don't necessarily have to bundle when building for node/bundlers. Only for the browser if you're targeting the option to making it usable as a script.