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

"moduleResolution": "node12" of Typescript 4.5 does not work as expected #46408

Closed
JounQin opened this issue Oct 18, 2021 · 19 comments
Closed

"moduleResolution": "node12" of Typescript 4.5 does not work as expected #46408

JounQin opened this issue Oct 18, 2021 · 19 comments

Comments

@JounQin
Copy link

JounQin commented Oct 18, 2021

I just tried the typescript@next and moduleResolution: 'node12' with @angular/compiler@v13, but tsc -b failed to build:

src/index.ts:1:15 - error TS2305: Module '"@angular/compiler"' has no exported member 'ParsedTemplate'.

1 import type { ParsedTemplate } from '@angular/compiler'
                ~~~~~~~~~~~~~~

src/index.ts:1:37 - error TS1471: Module '@angular/compiler' 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 { ParsedTemplate } from '@angular/compiler'
                                      ~~~~~~~~~~~~~~~~~~~

src/index.ts:2:30 - error TS7016: Could not find a declaration file for module 'synckit'. '/Users/JounQin/Workspaces/Local/test/node_modules/synckit/lib/index.cjs' implicitly has an 'any' type.
  Try `npm i --save-dev @types/synckit` if it exists or add a new declaration (.d.ts) file containing `declare module 'synckit';`

2 import { createSyncFn } from 'synckit'
                               ~~~~~~~~~

src/worker.ts:1:15 - error TS2305: Module '"@angular/compiler"' has no exported member 'ParsedTemplate'.

1 import type { ParsedTemplate } from '@angular/compiler'
                ~~~~~~~~~~~~~~

src/worker.ts:1:37 - error TS1471: Module '@angular/compiler' 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 { ParsedTemplate } from '@angular/compiler'
                                      ~~~~~~~~~~~~~~~~~~~

src/worker.ts:2:29 - error TS7016: Could not find a declaration file for module 'synckit'. '/Users/JounQin/Workspaces/Local/test/node_modules/synckit/lib/index.cjs' implicitly has an 'any' type.
  Try `npm i --save-dev @types/synckit` if it exists or add a new declaration (.d.ts) file containing `declare module 'synckit';`

2 import { runAsWorker } from 'synckit'
                              ~~~~~~~~~

src/worker.ts:5:11 - error TS2339: Property 'parseTemplate' does not exist on type 'typeof import("/Users/JounQin/Workspaces/Local/test/node_modules/@angular/compiler/index")'.

5   const { parseTemplate } = await import('@angular/compiler')
            ~~~~~~~~~~~~~


Found 7 errors.

@angular/compiler@v13 is ESM only, ParsedTemplate is typing exported from its types entry.

See https://unpkg.com/browse/@angular/compiler@13.0.0-rc.0/package.json

synckit is both commonjs and ESM compatible.

See https://unpkg.com/browse/synckit@0.6.0/package.json

I have no idea how can it be fixed on my side.


Test source codes:

// worker.ts
import type { ParsedTemplate } from '@angular/compiler'
import { runAsWorker } from 'synckit'

runAsWorker<ParsedTemplate>(async (code: string, filePath: string) => {
  const { parseTemplate } = await import('@angular/compiler')
  return parseTemplate(code, filePath, {
    preserveWhitespaces: true,
    preserveLineEndings: true,
    collectCommentNodes: true,
  })
})

Originally posted by @JounQin in #45884 (comment)

@MartinJohns
Copy link
Contributor

MartinJohns commented Oct 18, 2021

Issue template for bug reports. Although this looks more like a question, better suited for sites like StackOverflow.

@JounQin
Copy link
Author

JounQin commented Oct 18, 2021

It is created by Reference in new issue feature of GitHub from #45884 (comment).

@weswigham
Copy link
Member

synckit has a lib/index.d.ts which matches up to lib/index.js - its esm entrypoint. However, you're importing from a cjs mode file, so the exports map isn't resolving to that, it's resolving to lib/index.cjs instead, and there's no lib/index.d.cts to describe it, hence the error. synckit can either provide a lib/index.d.cts with its' cjs entrypoint shape, or it can provide a types condition that overrides the types we look up.

@angular/compiler is esm only - your errors tell me you're importing it in a cjs mode file. So that's an issue. Second, @angular/compiler is borked. What do I mean by that? Their declaration files are esm mode (as indicated by package.json, but their internal imports don't include any extensions! Their index.d.ts is just export * from "./compiler" - "./compiler" doesn't resolve under esm resolution rules, hence why there's no parseTemplate member. Whoever went and added the types condition to their export map didn't actually check that their types were esm resolution compatible.

@JounQin
Copy link
Author

JounQin commented Oct 19, 2021

@weswigham Thanks for reply.

I tried the following locally for synckit in node_modules:

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

This still does not work, is there anything wrong?

src/index.ts:2:30 - error TS1471: Module 'synckit' cannot be imported using this construct. The specifier only resolves to an ES module, which cannot be imported synchronously. Use dynamic import instead.

2 import { createSyncFn } from 'synckit'
                               ~~~~~~~~~

src/worker.ts:2:29 - error TS1471: Module 'synckit' cannot be imported using this construct. The specifier only resolves to an ES module, which cannot be imported synchronously. Use dynamic import instead.

2 import { runAsWorker } from 'synckit'
                              ~~~~~~~~~

@angular/compiler is esm only - your errors tell me you're importing it in a cjs mode file. So that's an issue.

But I'm using await import() in commonjs.

Did you mean something like export * from "./compiler" - "./compiler.js" is required?

@JounQin
Copy link
Author

JounQin commented Nov 4, 2021

I tried to use worker.cts instead of worker.ts with "type": "module" today, the errors from synckit reduced, but there is still:

src/worker.cts:1:29 - error TS1471: Module 'synckit' 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 { runAsWorker } from 'synckit'

I've tried the following in node_modules/synckit/package.json

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

I still don't understand how to fix this part.

See https://github.com/JounQin/test/tree/ts_esm for reproduction

cc @weswigham @DanielRosenwasser

@weswigham
Copy link
Member

types must appear above the other conditions in the exports map.

@weswigham
Copy link
Member

(But actually, since there's a separate cjs and esm entry point, I'd delete the types condition entirely and just add an index.d.cts)

@andrewbranch
Copy link
Member

I've made a note to discuss options for validating / issuing suggestions/diagnostics on package.json files, because that seems like a very easy mistake to make.

In the meantime, it sounds like things are working as expected here. @weswigham correct me if I’m wrong, but I think we can close this? @JounQin questions/discussion is welcome to continue, just trying to parse this to see if we need to assign out any immediate work.

@andrewbranch
Copy link
Member

More direct discussion of export map priorities being a footgun at #46334

@weswigham
Copy link
Member

Yeah, I don't think there's anything actionable for us here.

@JounQin
Copy link
Author

JounQin commented Nov 5, 2021

types must appear above the other conditions in the exports map.

@andrewbranch @weswigham

I tried

{
  "main": "./lib/index.cjs",
  "module": "./lib/index.js",
  "exports": {
    "types": "./lib/index.d.ts",
    "import": "./lib/index.js",
    "require": "./lib/index.cjs"
  },
  "types": "./lib/index.d.ts"
}

But it still does not work.

So index.d.cts seems to be the only option? But how can I produce index.d.cts with index.d.ts at the same time with tsc only?

rupurt added a commit to fremantle-industries/workbench that referenced this issue Dec 3, 2021
@RebeccaStevens
Copy link

RebeccaStevens commented May 18, 2022

@weswigham

Yeah, I don't think there's anything actionable for us here.

From 4.7 rc's release notes (for refernce)
// package.json
{
    "name": "my-package",
    "type": "module",
    "exports": {
        ".": {
            // Entry-point for `import "my-package"` in ESM
            "import": {
                // Where TypeScript will look.
                "types": "./types/esm/index.d.ts",

                // Where Node.js will look.
                "default": "./esm/index.js"
            },
            // Entry-point for `require("my-package") in CJS
            "require": {
                // Where TypeScript will look.
                "types": "./types/commonjs/index.d.cts",

                // Where Node.js will look.
                "default": "./commonjs/index.cjs"
            }
        }
    },

    // Fall-back for older versions of TypeScript
    "types": "./types/index.d.ts",

    // CJS fall-back for older versions of Node.js
    "main": "./commonjs/index.cjs"
}

It seems that all exports CJS exports using the .cjs extension must have their own type declaration files with a corresponding extensions. So every .cjs export must have an accompanying .d.cts; They cannot use a .d.ts file.

So the following doesn't work:

"exports": {
    ".": {
        "types": "./index.d.ts",
        "import": "./index.mjs",
        "require": "./index.cjs"
    }
}

Nor does this

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

Which means that if you want to use the same types for both your ESM and CJS exports, you'll need to make a dummy type file like this:

export * from "./index";
export { default } from "./index";

Edit: That work around doesn't work. You'd need to fully dupe the whole file and all sub files.

Wouldn't it be nicer to just allow .d.ts files to be used with .cjs files?

@weswigham
Copy link
Member

Nope! The format of the file is important information to the type system - it tells us how the module is loaded and, importantly, if there's a default that's the shape of the module itself when imported in esm, and if it's an error to load it at all in cjs. At runtime a file can't be both an esm and cjs module, and thus neither can the types for a module. (Though, as you've observed, nothing stops you from pulling almost the whole definition of one of the formats from the other one, in the same way you can re-export the actual implementation!)

@RebeccaStevens
Copy link

Though, as you've observed, nothing stops you from pulling almost the whole definition of one of the formats from the other one

I tried this and all though TS no longer complains about the initial import; within that file TS complains about the imports and refuses to resolve them, so you just end up with any types.

@weswigham
Copy link
Member

weswigham commented May 19, 2022

within that file TS complains about the imports and refuses to resolve them, so you just end up with any types.

Is that not just because node es module imports require full paths and explicit file extensions (likely .js) to resolve?

@RebeccaStevens
Copy link

RebeccaStevens commented May 20, 2022

Is that not just because node es module imports require full paths and explicit file extensions (likely .js) to resolve?

So I have the following types pre 4.7:

// index.d.ts
export { default } from "./foo";
export * from "./bar"; // type only exports

I tried making the following types for 4.7 to go along side them:

// index.d.mts
export { default } from "./index.js";
export * from "./index.js";
// index.d.cts
import Foo from "./index.js";
export = Foo;

But when using TypeScript 4.7-rc, TS complains about the exports (imports) in index.d.ts (due to not having a .js extensions). Is it possible to keep index.d.ts compatible with old versions of TypeScript that don't support the .js extension? Or will I have to duplicate all the type files to support both current and legacy type resolving? (Or will all relatively recent TS version understand the extension?)

Also, when using .cjs and .mjs files in vs-code insiders, TypeScript successfully finds all the types and doesn't complain about the missing extensions; that's only happening in .cts and .mts files. (I have tsconfig.json and jsconfig.json both setting "moduleResolution": "Node16").

On a sidenote, is it possible for me to re-export the type only exports in the .d.cts file while keeping export = Foo? - Actually this probably isn't necessary as types can't be imported via require.

@weswigham
Copy link
Member

Is it possible to keep index.d.ts compatible with old versions of TypeScript that don't support the .js extension?

.ts (and .d.ts) files adopt the same mode as .js files in their given context, so in a type: module folder, they'll be interpreted as esm (and thus require extensions on imports in them). You can use a typesVersions package override (or versioned types export map conditions) to provide a specific entry point for older TS (or newer TS) which does not (or does) respect that.

@weswigham
Copy link
Member

weswigham commented May 21, 2022

Or will all relatively recent TS version understand the extension?

But every version of ts ever (that has a commonjs resolver) actually allows keeping the (js) extension on the import, so you probably don't need to do much other than add extensions.

@RebeccaStevens
Copy link

RebeccaStevens commented May 21, 2022

Oki, that's good to know. It my be worth mentioning that in 4.7's release notes as I imagine quite a few other people will also have this misunderstanding as up until now, extensions have pretty much always been left off for TS/JS file imports.

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

5 participants