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

Cannot import a package with "type": "module" from cjs even if the package has an exports map #49299

Closed
RebeccaStevens opened this issue May 29, 2022 · 11 comments

Comments

@RebeccaStevens
Copy link

Bug Report

🔎 Search Terms

  • type: module
  • node16
  • The specifier only resolves to an ES module, which cannot be imported synchronously.

🕗 Version & Regression Information

I was unable to test this on prior versions because it's a brand new feature.

⏯ Playground Link

N/A

💻 Code

Code:

import foo from "my-dep";

tsconfig.json:

{
  "compilerOptions": {
    "module": "Node16"
  }
}

Dependency's package.json:

{
  "name": "my-dep",
  "type": "module",
  "exports": {
    "types": "index.d.ts",
    "import": "index.mjs",
    "require": "index.cjs"
  }
}

🙁 Actual behavior

Module 'my-dep' cannot be imported using this construct. The specifier only resolves to an ES module, which cannot be imported synchronously. Use dynamic import instead. ts(1471)

🙂 Expected behavior

TypeScript should be able to find the right import of my-dep from its exports map. The type field shouldn't prevent this.

Extra info

If I do any one of the following, everything works as expected.

  • specify "type": "module" in my own package.json
  • remove "type": "module" from my-dep's package.json
  • change my code's file extension to .mts

No changes to tsconfig.json seem to fix the issue.
I would have expected that the following would have worked but it didn't:

{
  "compilerOptions": {
    "module": "ESNext",
    "moduleResolution": "Node16"
  }
}
@Jamesernator
Copy link

Jamesernator commented May 29, 2022

So the issue happening here is that the line "types": "./index.d.ts" declares a types file that is inferred as an ESM module because of the "type": "module", i.e. because of "type": "module" the fact that ./index.d.ts ends with .ts means it gets treated as an ESM module declaration file.

An alternative fix (for the library) is to make ./index.d.ts a .cts file instead, which would be importable by both. Alternatively just providing types for the esm/cjs entry points separately would work i.e.:

"types": {
   "require": "./index.d.cts",
   "import": "./index.d.mts"
}

While it does seem like it would be reasonable for "types": "./index.d.ts" to apply for both, consider the following declaration:

export default function foo(): number;

If this declaration is interpreted as ESM, the intention is clear, however interpreted as CJS, is this supposed to be a CJS module with { default: () => number }? OR is the intention that this behaves like module.exports = foo (in TS syntax export = function foo(): number?

Both interpretations seem reasonable, and there's no way to tell just from the declaration which it is. If the library is actually implemented in TS, then the better thing for the library to do is just to have index.mts and index.cts and let TS generate the index.d.mts and index.d.cts files, if compiled into the same directory they'll be picked up just fine (or if a different directory, use "types": { "import": "...", "require": "..." } as above).

@RebeccaStevens
Copy link
Author

If I hover over foo in the import statement, in vscode; I do get the correct types.

@RebeccaStevens
Copy link
Author

is this supposed to be a CJS module with { default: () => number }? OR is the intention that this behaves like module.exports = foo (in TS syntax export = function foo(): number

TypeScript should just treat it as { default: () => number } as not doing so would break a lot of existing libraries (as this is how things have always worked, up until TS 4.7)

@Jamesernator
Copy link

Jamesernator commented May 29, 2022

If I hover over foo in the import statement, in vscode; I do get the correct types.

Note that from TS's perspective, there is no CJS module, this is because when "types" is specified TS will look through "exports" find that "my-dep" has a "types": "./index.d.ts" and stop searching.

Even though import foo from "my-dep" is technically require, from TS's perspective the resolution to this is still "./index.d.ts" because it the earliest match for importConditions = ["types", "require", "default"], basically the first entry in "exports" whose keys are all in the given conditions will win. (This is the same reason why if you put "default" first in "exports" it will always win regardless of other entries in "exports".)

Hence it shows you the types for the ES module, but it considers it an error as require will refuse to evaluate ESM which TS thinks is what is going to be require-ed based on the inferred "types" it found.

@RebeccaStevens
Copy link
Author

RebeccaStevens commented May 29, 2022

So basically the issue is that by the dependency specifying "type": "module", the .d.ts file will be interpreted as a module and thus cjs files cannot import it.
I assume the same thing would happen but in reverse if "type": "commonjs" was specified?

So if "type" is not specified, then the .d.ts file is free to be interpreted either way? Thus why removing "type" from the dependency seems to fix the issue.

If this is indeed the case, is it consider safe to take this approach or should a library always give both a .d.mts and a .d.cts file, even if the two files would be identical?

@Jamesernator
Copy link

Jamesernator commented May 29, 2022

So if "type" is not specified, then the .d.ts file is free to be interpreted either way? Thus why removing "type" from the dependency seems to fix the issue.

No then it will be interpreted as commonjs, however ESM can import so there won't be an error reported.

However, this isn't without hazards, i.e. consider if we had like I suggested above, however we also had an ESM file:

import foo from "my-dep";

console.log(foo);

Under the CJS resolution the shape of the module is { default: () => number }, so this means the program should prints:

{ default: function foo() { ... } }

This is probably not what was intended, because in ESM we would want the program to print the function itself:

function foo() { ... }

(Note that this is actually what the runtime does, because there actually is an ESM file).

This is unlike prior TS code, because in prior TS code there was esModuleInterop, which would've extracted the default export from the CJS namespace. But actual Node ESM doesn't extract the default export, it always returns the whole CJS namespace.

If this is indeed the case, is it consider safe to take this approach or should a library always give both a .d.mts and a .d.cts file, even if the two files would be identical?

Yes. It is kind've a shame that the files would be identical, although given identical contents is ideal for compression there's not really any loss to it.

@RebeccaStevens
Copy link
Author

RebeccaStevens commented May 29, 2022

Oki, that makes sense.

What about in the case of a JS library providing it's own type declarations?

Would the following be valid?

index.d.ts:

import type { Foo } from './types/foo.js';
import { Bar } from './types/bar.js';

export {
  Foo,
  Bar as default,
};

index.d.mts:

export { Foo } from "./index.js";
export default from "./index.js";

index.d.cts:

import Bar from "./index.js";
export = Bar;

Or would all the .d.ts files need to be converted to .d.mts and .d.cts?

@Jamesernator
Copy link

Would the following be valid?

Yes, this pattern is fine.

@RebeccaStevens
Copy link
Author

@Jamesernator from my testing, it seems to fail. With #47807, I believe it's possible to make it work but I haven't tested too much.

Also with regard to index.d.cts in my last comment, is it possible to also export the type Foo while using export = Bar?

@Jamesernator
Copy link

Jamesernator commented May 30, 2022

With #47807, I believe it's possible to make it work but I haven't tested too much.

Ah yeah, you probably need resolution-mode for this pattern.

Also with regard to index.d.cts in my last comment, is it possible to also export the type Foo while using export = Bar?

Kind've, you can export = a namespace and it'll allow type exports in that namespace. This may require changes to Bar, but namespace can generally merge with most kinds.

As an example if the CJS module.exports was a function then you can do:

declare function Bar(): number;
declare namespace Bar {
    export type Foo = {};
}

export = Bar;

And this will allow Bar to be both a function and a namespace with Bar.Foo as a type. (And will even work with import { Foo } from "..." in both ESM and CJS).

@RebeccaStevens
Copy link
Author

@Jamesernator Thanks for your help with this.

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

No branches or pull requests

2 participants