Module resolution: `resolution-mode="require"` emitted too eagerly

[Andarist]

Demo Repo

https://github.com/jaredwray/cacheable/tree/d7ac062f77bf8188221f914fd753122fd5aa5f65/packages/request

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.

Input

// @module: Node16
// @moduleResolution: Node16
// @declaration: true

/// <reference types="node" />

import {URL} from 'node:url';
import {EventEmitter} from 'node:events';
import {Buffer} from 'node:buffer';

export const stuff: [URL?, EventEmitter?, Buffer?] = [new URL('https://www.typescriptlang.org/play')]

emitted DTS

/// <reference types="node" resolution-mode="require"/>
/// <reference types="node" resolution-mode="require"/>
/// <reference types="node" resolution-mode="require"/>
import { URL } from 'node:url';
import { EventEmitter } from 'node:events';
import { Buffer } from 'node:buffer';
export declare const stuff: [URL?, EventEmitter?, Buffer?];

Run tsc --showConfig and paste its output here

importing module

{
    "compilerOptions": {
        "target": "esnext",
        "module": "commonjs",
        "noEmit": true,
        "isolatedModules": true,
        "strict": true,
        "esModuleInterop": true
    },
    "files": [
        "./packages/cli/dist/manypkg-cli.cjs.d.ts",
        "./packages/cli/script-runner/dist/cli.cjs.js.ts",
        "./packages/cli/src/errors.ts",
        "./packages/cli/src/index.ts",
        "./packages/cli/src/logger.ts",
        "./packages/cli/src/npm-tag.ts",
        "./packages/cli/src/run.test.ts",
        "./packages/cli/src/run.ts",
        "./packages/cli/src/test-helpers.ts",
        "./packages/cli/src/upgrade.ts",
        "./packages/cli/src/utils.ts",
        "./packages/cli/src/checks/EXTERNAL_MISMATCH.ts",
        "./packages/cli/src/checks/INCORRECT_REPOSITORY_FIELD.ts",
        "./packages/cli/src/checks/INTERNAL_MISMATCH.ts",
        "./packages/cli/src/checks/INVALID_DEV_AND_PEER_DEPENDENCY_RELATIONSHIP.ts",
        "./packages/cli/src/checks/INVALID_PACKAGE_NAME.ts",
        "./packages/cli/src/checks/MULTIPLE_DEPENDENCY_TYPES.ts",
        "./packages/cli/src/checks/ROOT_HAS_DEV_DEPENDENCIES.ts",
        "./packages/cli/src/checks/UNSORTED_DEPENDENCIES.ts",
        "./packages/cli/src/checks/index.ts",
        "./packages/cli/src/checks/utils.ts",
        "./packages/cli/src/checks/__tests__/EXTERNAL_MISMATCH.ts",
        "./packages/cli/src/checks/__tests__/INCORRECT_REPOSITORY_FIELD.ts",
        "./packages/cli/src/checks/__tests__/INTERNAL_MISMATCH.ts",
        "./packages/cli/src/checks/__tests__/INVALID_DEV_AND_PEER_DEPENDENCY.ts",
        "./packages/find-root/dist/manypkg-find-root.cjs.d.ts",
        "./packages/find-root/src/index.test.ts",
        "./packages/find-root/src/index.ts",
        "./packages/get-packages/dist/manypkg-get-packages.cjs.d.ts",
        "./packages/get-packages/src/index.test.ts",
        "./packages/get-packages/src/index.ts",
        "./packages/tools/dist/manypkg-tools.cjs.d.ts",
        "./packages/tools/src/BoltTool.ts",
        "./packages/tools/src/LernaTool.ts",
        "./packages/tools/src/PnpmTool.ts",
        "./packages/tools/src/RootTool.ts",
        "./packages/tools/src/RushTool.ts",
        "./packages/tools/src/Tool.ts",
        "./packages/tools/src/YarnTool.ts",
        "./packages/tools/src/expandPackageGlobs.ts",
        "./packages/tools/src/index.ts",
        "./types/fixturez.d.ts",
        "./types/spawndamnit.d.ts"
    ]
}

Run tsc --traceResolution and paste its output here

I think it would obscure this repro case - but I can add this information if requested.

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

{
  "name": "@manypkg/cli",
  "version": "0.21.0",
  "repository": {
    "type": "git",
    "url": "https://github.com/Thinkmill/manypkg.git",
    "directory": "packages/cli"
  },
  "license": "MIT",
  "main": "dist/manypkg-cli.cjs.js",
  "module": "dist/manypkg-cli.esm.js",
  "bin": {
    "manypkg": "./bin.js"
  },
  "dependencies": {
    "@manypkg/get-packages": "^2.2.0",
    "chalk": "^2.4.2",
    "detect-indent": "^6.0.0",
    "find-up": "^4.1.0",
    "fs-extra": "^8.1.0",
    "normalize-path": "^3.0.0",
    "p-limit": "^2.2.1",
    "package-json": "^8.1.0",
    "parse-github-url": "^1.0.2",
    "sembear": "^0.5.0",
    "semver": "^6.3.0",
    "spawndamnit": "^2.0.0",
    "validate-npm-package-name": "^3.0.0"
  },
  "devDependencies": {
    "fixturez": "^1.1.0",
    "strip-ansi": "^6.0.0"
  },
  "engines": {
    "node": ">=14.18.0"
  }
}

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

{
	"name": "cacheable-request",
	"version": "10.2.14",
	"description": "Wrap native HTTP requests with RFC compliant cache support",
	"license": "MIT",
	"repository": "jaredwray/cacheable",
	"author": "Jared Wray <me@jaredwray.com> (http://jaredwray.com)",
	"type": "module",
	"exports": "./dist/index.js",
	"types": "./dist/index.d.ts",
	"engines": {
		"node": ">=14.16"
	},
	"scripts": {
		"test": "xo && NODE_OPTIONS=--experimental-vm-modules jest --coverage ",
		"prepare": "npm run build",
		"build": "tsc --project tsconfig.build.json",
		"clean": "rm -rf node_modules && rm -rf ./coverage && rm -rf ./test/testdb.sqlite && rm -rf ./dist"
	},
	"files": [
		"dist"
	],
	"keywords": [
		"HTTP",
		"HTTPS",
		"cache",
		"caching",
		"layer",
		"cacheable",
		"RFC 7234",
		"RFC",
		"7234",
		"compliant"
	],
	"dependenciesComments": {
		"@types/http-cache-semantics": "It needs to be in the dependencies list and not devDependencies because otherwise projects that use this one will be getting `Could not find a declaration file for module 'http-cache-semantics'` error when running `tsc`, see https://github.com/jaredwray/cacheable-request/issues/194 for details"
	},
	"dependencies": {
		"@types/http-cache-semantics": "^4.0.4",
		"get-stream": "^6.0.1",
		"http-cache-semantics": "^4.1.1",
		"keyv": "^4.5.4",
		"mimic-response": "^4.0.0",
		"normalize-url": "^8.0.0",
		"responselike": "^3.0.0"
	},
	"devDependencies": {
		"@keyv/sqlite": "^3.6.6",
		"@types/jest": "^29.5.8",
		"@types/node": "^20.9.0",
		"@types/responselike": "^1.0.3",
		"@types/sqlite3": "^3.1.9",
		"body-parser": "^1.20.2",
		"delay": "^6.0.0",
		"eslint": "^8.53.0",
		"eslint-plugin-jest": "^27.6.0",
		"express": "^4.18.2",
		"jest": "^29.7.0",
		"pify": "^6.1.0",
		"sqlite3": "^5.1.6",
		"ts-jest": "^29.1.1",
		"ts-jest-resolver": "^2.0.1",
		"ts-node": "^10.9.1",
		"typescript": "^5.2.2",
		"xo": "^0.56.0"
	},
	"jest": {
		"collectCoverageFrom": [
			"src/**/*.{ts,js}"
		],
		"extensionsToTreatAsEsm": [
			".ts"
		],
		"resolver": "ts-jest-resolver",
		"moduleFileExtensions": [
			"ts",
			"js"
		],
		"transform": {
			"^.+\\.(ts|tsx)$": [
				"ts-jest",
				{
					"tsconfig": "./tsconfig.build.json",
					"useESM": true
				}
			]
		},
		"testMatch": [
			"**/test/*.test.(ts|js)"
		],
		"testEnvironment": "node"
	},
	"xo": {
		"plugins": [
			"jest"
		],
		"extends": [
			"plugin:jest/recommended"
		],
		"rules": {
			"@typescript-eslint/triple-slash-reference": 0,
			"@typescript-eslint/no-namespace": 0,
			"@typescript-eslint/no-unsafe-assignment": 0,
			"@typescript-eslint/no-unsafe-call": 0,
			"@typescript-eslint/ban-types": 0,
			"@typescript-eslint/restrict-template-expressions": 0,
			"@typescript-eslint/no-unsafe-return": 0,
			"@typescript-eslint/no-unsafe-argument": 0,
			"new-cap": 0,
			"unicorn/no-abusive-eslint-disable": 0,
			"@typescript-eslint/restrict-plus-operands": 0,
			"@typescript-eslint/no-implicit-any-catch": 0,
			"@typescript-eslint/consistent-type-imports": 0,
			"@typescript-eslint/consistent-type-definitions": 0,
			"@typescript-eslint/prefer-nullish-coalescing": 0,
			"n/prefer-global/url": 0,
			"n/no-deprecated-api": 0,
			"unicorn/prefer-event-target": 0
		}
	}
}

Any other comments can go here

I included a dynamic import to this ESM-only package in my CJS module

export function getPackageInfo(pkgName: string) {
  return npmRequestLimit(async () => {
    const getPackageJson = (await import("package-json")).default;

    return getPackageJson(pkgName, {
      allVersions: true,
    });
  });
}

that generated its bit of a declaration file:

export declare function getPackageInfo(pkgName: string): Promise<import("package-json").AbbreviatedMetadata>;

Overall, I understand that this is no longer a problem:

error TS1452: 'resolution-mode' assertions are only supported when `moduleResolution` is `node16` or `nodenext`.

4 /// <reference types="node" resolution-mode="require"/>

since TS 5.3:

Previously, using resolution-mode was only allowed under the moduleResolution options node16 and nodenext. To make it easier to look up modules specifically for type purposes, resolution-mode now works appropriately in all other moduleResolution options like bundler, node10, and simply doesn’t error under classic.

However, I don't understand why resolution-mode="require" was added there in the first place. That file was ESM and I imagine that it should go through its default resolution mode (import?) when referring to node builtin modules. After all, the runtime output looks roughly equivalent to smth like this:

/// <reference types="node" />
import { URL } from 'node:url';
export const stuff = [new URL('https://www.typescriptlang.org/play')];

Since this isn't a CJS module I think that its DTS output shouldn't try to "dereference" those references but rather should attempt to preserve the import style~ of the source.

cc @andrewbranch

[andrewbranch]

From #55579:

Presumably, the resolution-mode assertion gets added in declaration emit to increase the likelihood that, if the user installs a version of chai that contains separate ESM and CJS types, the ESM-format .d.ts file here will still resolve to the CJS types, as it did when it was created by the library’s compilation. (Note that if the user does have the same version of chai that the library had during compilation, the resolution-mode assertion will not actually be needed.)

This is my best guess, which means it’s working as intended, or at least, working as somebody intended. I’m not sure if that behavior is ideal. Do you have any real examples of it being incorrect?

[Andarist]

I'm not super sure how those triple slash directives work and when they are exactly needed etc - I almost never write them by hand. So take my opinion with a grain of salt etc.

The quoted statement reads to me a little bit like "we need to inline type aliases to increase the likelihood that the functions declared by the module keep accepting arguments of the exact same shape as they did when it was created by the library's compilation".

However, node types live kinda outside of the regular semver ranges governed by the dependency ranges... so I'm not sure. Maybe this is just a quirk of that? Are triple slash directives ever included in other scenarios (beyond when the library depends on some global-like declarations etc)?

But even with that in mind... the node types development is usually additive. They are often installed by the consuming project - acting like a peer dep of sorts. So I think, I would still expect the behavior as close as possible to the one that was authored. If the source code goes through the import resolver then I think it makes total sense for the output declarations to go through the same import resolver too. This semantic change~ of the output is surprising.

That being said - I didn't really notice this thing while investigating a broken case per se. I just noticed this because I tried to consume this package using TS 4.9 with moduleReoslution: node and... the compilation complained. So from that PoV keeping the original "intention" in the emitted declarations would improve compatibility with older TS versions, the emitted declaration would be more portable.

As long as this extra attribute is not needed - I think it's not worth adding it since it has a real chance of breaking some builds. TS 5.3 permits this in all module resolution modes but this is a very recent change and IIRC, for example, types in DefinitelyTyped support at least 2 years of the last TS releases.

I figured out it's something worth raising - even if only for this situation to findable by others in the issue tracker.

[andrewbranch]

They’re only emitted when the file references a global, ambient module, or exports from a module augmentation.

The rationale behind including the resolution-mode assertion on these is the same argument @weswigham made in #53426, so I think this is working as intended.

[Andarist]

Now suppose mypkg does the supposedly backwards-compatible thing and adds an explicit cjs entrypoint of the same shape
[...]
If we did this, we make what should be a non-breaking, safe change into a breaking change for TS consumers. That won't be good for the ecosystem. :(

Isn't at least this bit indicating that this issue here has some merit?

Scenario:

1. CJS consumer + dynamic import('esm-only')
2. esm-only imports CJS pkg and that emits some references with resolution-mode="require"
3. now CJS pkg adds ESM using the dual package strategy
4. IIUC, our esm-only package now refers to CJS types but ESM runtime values - that's potentially dangerous in some contrived examples

If those "redundant" resolution-mode="require" wouldn't be emitted then esm-only would stay "internally consistent" (a good thing), its runtime and type spaces wouldn't desynchronize and that desynchronization wouldn't "leak" into our CJS consumer.

I have a massive headache and thinking about modules doesn't soothe it 😅 I understand that there are just tradeoffs to be considered here, for almost every potential solution. Feel free to close this issue.

[andrewbranch]

For one, Wesley’s assertion is that when pkg goes from CJS to dual, it should do a semver major bump, but going from ESM-only to dual does not necessitate that. Secondly, we can’t say that the actual module being imported in esm-only is either ESM or CJS, because it’s typed by an ambient module declaration, which does not distinguish between ESM/CJS. So it’s not necessarily “CJS types but ESM runtime values”; it’s just that we only know about the existence of some module specifier by loading a file whose extension implied it was CJS.

It’s extremely hard to argue that this behavior is right or wrong, given that global declarations and ambient module declarations kind of sit outside the ESM/CJS world. Like, what is the actual scenario we think might happen where the resolution of this triple-slash reference would change?

[justshubham07]

// @module: Node16
// @moduleResolution: Node16
// @declaration: true

///

import { URL } from 'node:url';
import { EventEmitter } from 'node:events';
import { Buffer } from 'node:buffer';

export const stuff: [URL?, EventEmitter?, Buffer?] = [new URL('https://www.typescriptlang.org/play'), /* Add a comma here */];

There is a syntax error in the code you provided. The issue is in the line where you initialize the stuff array. There's a missing comma after the first element

[andrewbranch]

Closed by #57681 and #57867