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

error importing type from esm in commonjs #47338

Closed
nstringham opened this issue Jan 6, 2022 · 2 comments Β· Fixed by #47807
Closed

error importing type from esm in commonjs #47338

nstringham opened this issue Jan 6, 2022 · 2 comments Β· Fixed by #47807
Assignees
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status.

Comments

@nstringham
Copy link

Bug Report

πŸ”Ž Search Terms

typeof import
import esm type from commonjs module
node12 nodenext

πŸ•— Version & Regression Information

  • I was unable to test this on prior versions because "module": "node12" only works in typescript@next

⏯ Playground Link

I was unable to find a way to emulate "type": "commonjs" in package.json on typescript playground

here is a repo that can be used to reproduce this error
https://github.com/nstringham/min-to-reproduce-typescript-import-problem

πŸ’» Code

/tsconfig.json

{
  "compilerOptions": {
    "module": "nodenext"
  }
}

/dependency/index.d.ts

export type Color = "red" | "green" | "blue";

export function printColor(color: Color): void;

/index.ts

import type { Color } from "dependency";

const myColor: Color = "blue";

const myOtherColor: import("dependency").Color = "red";

async function main() {
  const { printColor } = await import("dependency");
  printColor(myColor);
  printColor(myOtherColor);
}

main();

πŸ™ Actual behavior

code compiles correctly but displays the following error

index.ts:1:28 - error TS1471: Module 'dependency' cannot be imported using this construct. The specifier only resolves to an ES module, which cannot be imported synchronously. Use dynamic import instead.

1 import type { Color } from "dependency";
                             ~~~~~~~~~~~~

index.ts:5:28 - error TS1471: Module 'dependency' cannot be imported using this construct. The specifier only resolves to an ES module, which cannot be imported synchronously. Use dynamic import instead.

5 const myOtherColor: import("dependency").Color = "red";
                             ~~~~~~~~~~~~

πŸ™‚ Expected behavior

code compiles correctly with no errors

β†ͺ️ Workaround

because typescript compiles correctly despite errors this error can be ignored with // @ts-ignore

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Jan 12, 2022
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.7.0 milestone Jan 12, 2022
@weswigham
Copy link
Member

weswigham commented Jan 18, 2022

So, my thoughts on this, since there have been a few issues in this vein (eg, #47248) and while I've spoken to them in team settings, I have yet to set them down.

This is technically working as expected right now, since you're importing an esm format files from a cjs one using constructs that check runtime behavior. You may think, "But @weswigham, I'm only importing the types, surely that means it's safe to just give me the type data, right!?!? There's no runtime to be concerned about!" And you'd be right, but only in the narrowest sense - if we were to allow pulling types from modules whose format is incompatible like this, it becomes a breaking change to then add a compatible format to that package, rather than a non-breaking one. Meaning we'd be making adding more format compatibility via conditional exports a breaking change, which defeats a core premise of conditional exports.

Therefore, I would not expect us to ever make the above work as written; instead, I would anticipate us adding a syntax to specifically get the esm or cjs format type information for a specifier even when that's not the default mode for the file's imports (like how you have dynamic import available in a cjs context to do esm resolution). Something like import("dependency", { assert: { mode: "esm" } }).Color, import type { Color } from "dependency" assert { mode: "esm" } or similar. Incredibly verbose, but one of the only ways I can think to do it without inadvertently introducing refactoring hazards or introducing bespoke syntax.

@nstringham
Copy link
Author

So, my thoughts on this, since there have been a few issues in this vein (eg, #47248) and while I've spoken to them in team settings, I have yet to set them down.

This is technically working as expected right now, since you're importing an esm format files from a cjs one using constructs that check runtime behavior. You may think, "But @weswigham, I'm only importing the types, surely that means it's safe to just give me the type data, right!?!? There's no runtime to be concerned about!" And you'd be right, but only in the narrowest sense - if we were to allow pulling types from modules whose format is incompatible like this, it becomes a breaking change to then add a compatible format to that package, rather than a non-breaking one. Meaning we'd be making adding more format compatibility via conditional exports a breaking change, which defeats a core premise of conditional exports.

Therefore, I would not expect us to ever make the above work as written; instead, I would anticipate us adding a syntax to specifically get the esm or cjs format type information for a specifier even when that's not the default mode for the file's imports (like how you have dynamic import available in a cjs context to do esm resolution). Something like import("dependency", { assert: { mode: "esm" } }).Color, import type { Color } from "dependency" assert { mode: "esm" } or similar. Incredibly verbose, but one of the only ways I can think to do it without inadvertently introducing refactoring hazards or introducing bespoke syntax.

makes sense. Thanks for looking into it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
4 participants