Skip to content

fix: improve type support for isolated dependencies in pnpm#289

Merged
mdjermanovic merged 11 commits intomainfrom
pnpm-test
Oct 17, 2025
Merged

fix: improve type support for isolated dependencies in pnpm#289
mdjermanovic merged 11 commits intomainfrom
pnpm-test

Conversation

@fasttime
Copy link
Member

@fasttime fasttime commented Oct 7, 2025

Prerequisites checklist

What is the purpose of this pull request?

This pull request, together with eslint/eslint#20201 in the main repo, fixes issue #283 by replacing import() syntax inside JSDoc @typedef tags with local module aliases in the bundled .js output.

Before:

/** @typedef {import("@eslint/core").ConfigObject} Config */
/** @typedef {import("@eslint/core").LegacyConfigObject} LegacyConfig */

After

/** @import * as $eslintcore from "@eslint/core"; */
/** @typedef {$eslintcore.ConfigObject} Config */
/** @typedef {$eslintcore.LegacyConfigObject} LegacyConfig */

This has the effect of creating a type import (import type *) in the generated .d.ts files:

Before:

export type Config = import("@eslint/core").ConfigObject;
export type LegacyConfig = import("@eslint/core").LegacyConfigObject;

After

export type Config = _eslintcore.ConfigObject;
export type LegacyConfig = $eslintcore.LegacyConfigObject;
import type * as $eslintcore from "@eslint/core";

The type import doesn't change the exported types, but it overcomes a limitation of TypeScript when resolving types across symlinked directories. See also microsoft/TypeScript#62558.

The current change only affects packages built using the dedupe-types tools:

  • config-array
  • config-helpers
  • plugin-kit

Anyway, these are not the only packages impacted by the TypeScript issue. compat is widely used and it is also impacted:

StackBlitz demo

This could be fixed in a follow-up pull request if necessary.

What changes did you make? (Give an overview)

  • Added tests for pnpm type support.
  • Updated dedupe-types and build-cts tools to match the new format.
  • Updated new package template.

Related Issues

fixes #283

Is there anything you'd like reviewers to focus on?

@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Oct 7, 2025
@fasttime fasttime changed the title test: add pnpm type support test fix: improve type support for isolated dependencies in pnpm Oct 8, 2025
@eslint-github-bot eslint-github-bot bot added the bug Something isn't working label Oct 8, 2025
"extends": "./tsconfig.json",
"files": ["dist/esm/index.js"],
"compilerOptions": {
"allowImportingTsExtensions": true,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allowImportingTsExtensions is required for TypeScript to accept import statements from "./types.ts" in the generated dist/esm/index.js file:

/** @import * as _typests from "./types.ts"; */

Another possibility to avoid allowImportingTsExtensions is importing from "./types.js" instead. This will work if a dummy file dist/esm/types.js is added to the package, similarly to what is done in eslint/markdown:

https://app.unpkg.com/@eslint/markdown@7.4.0/files/dist/esm/types.js

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of copying this into every tsconfig.esm.json file, can we add it to ./tsconfig.json?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added it to tsconfig.base.json, which is the base config extended by the various tsconfig.esm.json files in the package directories, via "./tconfig.json" < "../../tsconfig.base.json".

@nzakas
Copy link
Member

nzakas commented Oct 9, 2025

Is there a reason we can't do this?

/** @import { ConfigObject as Config, LegacyConfigObject as LegacyConfig ) from "@eslint/core" */

@nzakas nzakas moved this from Needs Triage to Implementing in Triage Oct 9, 2025
@fasttime
Copy link
Member Author

Is there a reason we can't do this?

/** @import { ConfigObject as Config, LegacyConfigObject as LegacyConfig ) from "@eslint/core" */

The problem is that @import tags only generate import statements, not the corresponding exports. That is why @typedef is needed.

For example, this JSDoc comment:

/** @import { ConfigObject as Config, LegacyConfigObject as LegacyConfig, Plugin, RuleConfig } from "@eslint/core"; */

Compiles into:

import type { ConfigObject as Config } from "@eslint/core";

In this case, TypeScript even recognizes that LegacyConfig, Plugin and RuleConfig are unused and prunes them from the generated types.

nzakas
nzakas previously approved these changes Oct 17, 2025
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one suggestion and otherwise LGTM.

"extends": "./tsconfig.json",
"files": ["dist/esm/index.js"],
"compilerOptions": {
"allowImportingTsExtensions": true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of copying this into every tsconfig.esm.json file, can we add it to ./tsconfig.json?

@nzakas nzakas moved this from Implementing to Merge Candidates in Triage Oct 17, 2025
@mdjermanovic
Copy link
Member

There's a merge conflict in packages/object-schema/package.json now.

@fasttime
Copy link
Member Author

There's a merge conflict in packages/object-schema/package.json now.

It's fixed now after rebasing.

@mdjermanovic
Copy link
Member

@fasttime which packages should be released after this change? Per the diff, it looks like this would only trigger config-helpers and object-schema?

@fasttime
Copy link
Member Author

@fasttime which packages should be released after this change? Per the diff, it looks like this would only trigger config-helpers and object-schema?

I think the packages affected by the changes in tools/dedupe-types.js and tools/build-cts.js are compat, config-array, config-helpers, object-schema and plugin-kit.

@fasttime
Copy link
Member Author

To fix just #283, config-array, config-helpers and object-schema should be sufficient.

@mdjermanovic
Copy link
Member

To fix just #283, config-array, config-helpers and object-schema should be sufficient.

Alright, since all 3 are already included in #296, I'm going to merge this and release these three packages.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've verified that @nzakas's suggestion has been applied.

@mdjermanovic mdjermanovic merged commit f8df139 into main Oct 17, 2025
25 checks passed
@mdjermanovic mdjermanovic deleted the pnpm-test branch October 17, 2025 17:47
@github-project-automation github-project-automation bot moved this from Merge Candidates to Complete in Triage Oct 17, 2025
@github-actions github-actions bot mentioned this pull request Oct 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working chore

Projects

Status: Complete

3 participants