Module resolution: d.ts files do not get treated depending on the importer's type in package.json, causing issues with resolution-modes

[vladfrangu]

Demo Repo

https://github.com/vladfrangu/ts-resolution-issue

Which of the following problems are you reporting?

Something else more complicated which I'll explain in more detail

Demonstrate the defect described above with a code sample.

import { API } from '@discordjs/core/http-only';

// vvv switch these lines around and see it working
import { REST } from 'discord.js';
// import { REST } from '@discordjs/rest';
// ^^^ switch these lines around and see it working

const rest = new REST({ version: '10' }).setToken('');
const api = new API(rest);

Run tsc --showConfig and paste its output here

{
    "compilerOptions": {
        "allowUnreachableCode": false,
        "allowUnusedLabels": false,
        "exactOptionalPropertyTypes": true,
        "noFallthroughCasesInSwitch": true,
        "noImplicitOverride": true,
        "noImplicitReturns": true,
        "noUncheckedIndexedAccess": true,
        "noUnusedLocals": true,
        "noUnusedParameters": true,
        "strict": true,
        "module": "nodenext",
        "moduleResolution": "nodenext",
        "resolveJsonModule": true,
        "verbatimModuleSyntax": true,
        "newLine": "lf",
        "outDir": "./distribution",
        "esModuleInterop": true,
        "forceConsistentCasingInFileNames": true,
        "lib": [
            "esnext"
        ],
        "target": "esnext",
        "skipLibCheck": true
    },
    "files": [
        "./index.ts"
    ],
    "exclude": [
        "distribution"
    ]
}

Run tsc --traceResolution and paste its output here

See https://github.com/vladfrangu/ts-resolution-issue/blob/main/resolutions.txt (couldn't paste it due to size)

Paste the package.json of the importing module, if it exists

{
  "type": "module",
  "scripts": {
    "test": "tsc --noEmit"
  },
  "dependencies": {
    "@discordjs/core": "^1.1.1",
    "discord.js": "^14.14.1"
  },
  "devDependencies": {
    "typescript": "^5.2.2"
  }
}

Paste the package.json of the target module, if it exists

{
  "$schema": "https://json.schemastore.org/package.json",
  "name": "discord.js",
  "version": "14.14.1",
  "description": "A powerful library for interacting with the Discord API",
  "main": "./src/index.js",
  "types": "./typings/index.d.ts",
  "directories": {
    "lib": "src",
    "test": "test"
  },
  "files": [
    "src",
    "typings"
  ],
  "contributors": [
    "Crawl <[email protected]>",
    "Amish Shah <[email protected]>",
    "Vlad Frangu <[email protected]>",
    "SpaceEEC <[email protected]>",
    "Aura Román <[email protected]>"
  ],
  "license": "Apache-2.0",
  "keywords": [
    "discord",
    "api",
    "bot",
    "client",
    "node",
    "discordapp"
  ],
  "repository": {
    "type": "git",
    "url": "https://github.com/discordjs/discord.js.git",
    "directory": "packages/discord.js"
  },
  "bugs": {
    "url": "https://github.com/discordjs/discord.js/issues"
  },
  "homepage": "https://discord.js.org",
  "dependencies": {
    "@discordjs/collection": "1.5.3",
    "@sapphire/snowflake": "3.5.1",
    "@types/ws": "8.5.9",
    "discord-api-types": "0.37.61",
    "fast-deep-equal": "3.1.3",
    "lodash.snakecase": "4.1.1",
    "tslib": "2.6.2",
    "undici": "5.27.2",
    "ws": "8.14.2",
    "@discordjs/builders": "^1.7.0",
    "@discordjs/formatters": "^0.3.3",
    "@discordjs/util": "^1.0.2",
    "@discordjs/rest": "^2.1.0",
    "@discordjs/ws": "^1.0.2"
  },
  "devDependencies": {
    "@favware/cliff-jumper": "2.2.1",
    "@types/node": "16.18.60",
    "@typescript-eslint/eslint-plugin": "^6.10.0",
    "@typescript-eslint/parser": "^6.10.0",
    "cross-env": "^7.0.3",
    "dtslint": "4.2.1",
    "eslint": "8.53.0",
    "eslint-formatter-pretty": "5.0.0",
    "jest": "29.7.0",
    "prettier": "3.0.3",
    "tsd": "0.29.0",
    "tslint": "6.1.3",
    "turbo": "^1.10.17-canary.0",
    "typescript": "5.2.2",
    "@discordjs/api-extractor": "^7.38.1",
    "@discordjs/docgen": "^0.12.1"
  },
  "engines": {
    "node": ">=16.11.0"
  },
  "scripts": {
    "test": "pnpm run docs:test && pnpm run test:typescript",
    "test:typescript": "tsc --noEmit && tsd",
    "lint": "prettier --check . && tslint typings/index.d.ts && cross-env ESLINT_USE_FLAT_CONFIG=false eslint --format=pretty src typings",
    "format": "prettier --write . && cross-env ESLINT_USE_FLAT_CONFIG=false eslint --fix --format=pretty src",
    "fmt": "pnpm run format",
    "docs": "docgen -i './src/*.js' './src/**/*.js' -c ./docs/index.json -r ../../ -o ./docs/docs.json && pnpm run docs:new",
    "docs:test": "docgen -i './src/*.js' './src/**/*.js' -c ./docs/index.json -r ../../",
    "docs:new": "api-extractor -d run --local",
    "changelog": "git cliff --prepend ./CHANGELOG.md -u -c ./cliff.toml -r ../../ --include-path 'packages/discord.js/*'",
    "release": "cliff-jumper"
  }
}

Any other comments can go here

At first, we assumed that the issue was that discord.js lacks an exports key, but as you can see by running node itShouldWorkWithThisOnly.mjs in the cloned repository (after installing dependencies), that isn't the case.

The only way to make the error not happen is by having an d.mts file and passing it into the import key for the exports (run itWorksNowTho.mjs in the cloned repository)

This used to not be needed. Before, TSC would treat d.ts files according to whatever export condition it matched (so, if it was in an import condition -> ESM, require -> CJS, etc). It seems that resolution-mode has made this much stricter, or possibly even outright broke behavior.

This has caused discordjs/discord.js#9985 and has required us to alter our build pipeline for our other packages (tsup used to generate a "shared" d.ts file which contained duplicated typing declarations, but ever since resolution became stricter (or broke? not sure), it was always interpreted as a CJS declaration instead of an universal one

[andrewbranch]

Not only is TypeScript working as intended here, the types for these packages appear to be plausibly correct, and warning you of a real instance of the dual-package hazard.

~/Developer/microsoft/eg/ts-resolution-issue (main) 
❯ node --input-type=module -e 'import { REST as R1 } from "discord.js"; import { RE
ST as R2 } from "@discordjs/rest"; console.log(R1 === R2)'
false

The types resolve to two different declarations of the class REST, one in ESM and one in CJS, and that’s literally what’s happening in Node.js too! The classes have private fields, which means the compiler has to treat them nominally. When API says its constructor needs an instance of the REST it’s referring to, the other REST class may not be substitutable for it because its private fields may have incompatible types.

If API does not need an exact REST instance but rather some object that conforms to an interface, it should say so. As it is, it says it needs exactly an instance of a particular class, and your example code is not giving it that.

Before, TSC would treat d.ts files according to whatever export condition it matched (so, if it was in an import condition -> ESM, require -> CJS, etc)

This has never been the case, and any implementation that did this while trying to model Node.js would be incorrect.

[vladfrangu]

This has never been the case, and any implementation that did this while trying to model Node.js would be incorrect

Interesting... this hasn't been an issue for a while, and only recently creeped up, I guess there were some changes in how file resolutions worked / how d.ts files were interpreted (compared to d.cts/d.mts)?

---

Also, do you have any tips on

(tsup used to generate a "shared" d.ts file which contained duplicated typing declarations, but ever since resolution became stricter (or broke? not sure), it was always interpreted as a CJS declaration instead of an universal one

Would that count as a separate issue? I could probably try to build an example module showcasing that issue in particular if needed 👀

[fatcerberus]

When API says its constructor needs an instance of the REST it’s referring to, the other REST class may not be substitutable for it because its private fields may have incompatible types.

Since we're dealing with #private, they don't even need to have incompatible types:

class A1 {
    #foo = 42;
    go(a) {
        a.#foo;
    }
}

class A2 {
    #foo = 42;
    go(a) {
        a.#foo;
    }
}

const a1 = new A1();
const a2 = new A2();
a1.go(a1);  // ok
a1.go(a2);  // runtime error
a2.go(a1);  // runtime error
a2.go(a2);  // ok

Which makes the dual-package hazard even more of a footgun.

[andrewbranch]

Interesting... this hasn't been an issue for a while, and only recently creeped up, I guess there were some changes in how file resolutions worked / how d.ts files were interpreted (compared to d.cts/d.mts)?

It has been possible to get yourself into this exact situation since TypeScript 4.7; minor things have been fixed and tweaked but I can’t think of anything that would have suddenly made this more likely for you. It seems more likely that those Discord libraries were updated in a way that made this blow up.

Also, do you have any tips on

(tsup used to generate a "shared" d.ts file which contained duplicated typing declarations, but ever since resolution became stricter (or broke? not sure), it was always interpreted as a CJS declaration instead of an universal one

Would that count as a separate issue? I could probably try to build an example module showcasing that issue in particular if needed 👀

There’s no such thing as a “universal” declaration file; tsup’s single-.d.ts output was always incorrect. I fixed it a few months ago by making it emit separate .d.mts and .d.cts files.

It looks like the @discord packages are all dual while discord.js package is CJS-only, and you’re writing an ESM app. Seems like it’s going to be very painful to try to use that combo together. Probably your best choices are either to stop using discord.js or switch your usage of all @discord packages to use require. This works, for example:

import http = require('@discordjs/core/http-only');
import discord = require('discord.js');

const rest = new discord.REST({ version: '10' }).setToken('');
const api = new http.API(rest);

[ckohen]

We're trying to fix this for consumers of discord.js. I've already mentioned directly importing /rest as a workaround for the moment.

There’s no such thing as a “universal” declaration file; tsup’s single-.d.ts output was always incorrect. I fixed it a few months ago by making it emit separate .d.mts and .d.cts files.

Technically no, realistically yes. The contents of the files are identical to this day, not to say that they shouldn't be separate files right now (specifically because of issues like you pointed out already). But in the PR where you fixed it by having separate outputs you neglected to ensure that any chunks generated also got this treatment. We had at least one release cycle that contained these chunks (a single .d.ts alongside our normal multiple entrypoints with d.mts and .d.ts) without these errors coming up.

Essentially, what the compiler should've seen was app imports @discordjs/core => index.d.mts => types-[hash].d.ts has import { REST } from '@discordjs/rest' => now points to /rest's index.d.ts instead of index.d.mts.
We've released a fix that just has multiple entrypoints compiled separately while waiting for tsup to release a version with the fixed chunks issue.

You are correct that for discord.js and discordjs/core this is a real dual package hazard. However, when an esm app imports from a dual package that imports from another dual package and it just so happens to encounter a chunked file in the first, since there is no actual difference in declaration files, why should we have to ship two versions of the chunked file?

[andrewbranch]

Because a declaration file informs the compiler about the existence of and module format of exactly one JavaScript file, and misrepresenting the module format of the JavaScript file will lead the compiler to issue incorrect errors or type imports incorrectly. Also, it’s how this exact dual package hazard issue was caught. If these libraries had tried to fudge the reality and share a single “universal” declaration of REST even though two separate ones exist at runtime, TypeScript would not have been able to determine that there was a problem. One JS declaration has one .d.[mc]?ts declaration; if you ship nearly duplicated JavaScript, you’re going to ship nearly duplicated declarations if you want it to be semantically correct. Sometimes that duplication has no observable effect; sometimes it does, like here.

[ckohen]

So what are we supposed to do for type only files? If a declaration file represents one js file, then type only files clearly mustn't exist. But hey, say we made one anyways, it works great up until you import a dual package into said file.

This seems random and is super unintuitive, especially considering the errors it generates look like typescript is saying the exact same thing is not assignable to itself (because most beginner-intermediate devs neglect the 3 words that indicate that the module resolution occurred differently in the massive error generated)

[fatcerberus]

You're not supposed to use .d.ts for type-only files. That's not considered a supported use case; compiler says .d.ts always corresponds to a .js file (to the point that you can import foo.d.ts as foo.js even if the latter file doesn't exist).

If your type-only .d.ts was generated from a .ts file by the compiler, then you still have a corresponding .js - even if it's empty.

[ckohen]

Not always. The typescript compiler, which of course doesn't support dual packages anyways, does this. A bundler will either completely remove the file from the output or will bundle it directly in as an empty file.
In our case, due to an enum being in the type-only file, the bundle does actually include some js code in this "file".
However, the js bundle is not guaranteed to chunk the same way the declaration bundle does. If it's supposed to, then line up an issue to all the bundlers cause I'd feel pretty confident guaranteeing not one of them does it correctly.

[fatcerberus]

The typescript compiler, which of course doesn't support dual packages anyways, does this.

I don't know what to tell you except that 1) it's been stated by the TS devs that .d.ts files are supposed to be in 1:1 correspondence with .js files (and that things will go sideways if that's violated) and 2) yes, .d.ts is a TypeScript concept so TypeScript's behavior is correct pretty much by definition.

With regard to hypothetical dual package support in TS itself, refer to #54593.

[andrewbranch]

While a module declaration file always implies the existence of a module JavaScript file, practically speaking, if you have a dual package and want to share type definitions between both formats, it’s currently a better choice to make them CommonJS format, because those can be easily imported by both formats. If you make your types-only file look like it represents an ESM JavaScript file, you will have to import it like this in your CommonJS package:

import type { Types } from "../shared/types.mts" with { "resolution-mode": "import" }

which is only supported in TypeScript 5.3+.

[andrewbranch]

If it's supposed to, then line up an issue to all the bundlers cause I'd feel pretty confident guaranteeing not one of them does it correctly.

Yes, now we’re on the same page https://www.typescriptlang.org/docs/handbook/modules/guides/choosing-compiler-options.html#considerations-for-bundling-libraries

I encourage people not to bundle libraries because of these issues. I’ve talked with Evan You about fixing this in Rolldown by giving it first-class declaration emit support. The problem with existing solutions is that declaration emit is being done totally separately from the bundler’s core work, so it’s not possible to coordinate them.

[typescript-bot]

This issue has been marked as "Working as Intended" and has seen no recent activity. It has been automatically closed for house-keeping purposes.