@typescript-eslint/types has a phantom dependency on the "typescript" package

[octogonz]

• I have tried restarting my IDE and the issue persists.
• I have updated to the latest version of the packages.
• I have read the FAQ and my problem is not listed.

Repro

1. My scenario involves an ESLint plugin project in a monorepo with side-by-side versions of TypeScript
2. Upgrade that project to use the latest @typescript-eslint/parser, with an indirect dependency on @typescript-eslint/types
3. A file imported from @typescript-eslint/types fails to compile if your project is not using the very latest release of TypeScript. The error looks like:

   @typescript-eslint/types/dist/ast-spec.d.ts:781:5 - (TS1169) A computed property 
   name in an interface must refer to an expression whose type is a literal type 
   or a 'unique symbol' type.
   

Expected: We can solve this problem by upgrading the TypeScript dependency for our ESLint plugin project, while other monorepo projects continue to use the older TypeScript compiler that they target.

Actual: Upgrading TypeScript for the the plugin package does NOT upgrade the TypeScript import for @typescript-eslint/types. As an indirect dependency, instead it accidentally picks up the older TypeScript version that was installed for a different project. This is because the package manager is not required to make any guarantees about the version (or even existence) of typescript for @typescript-eslint/types, because it was not declared in package.json.

Specifically, the typescript package is referenced here:

dist/ast-spec.d.ts

import type { SyntaxKind } from 'typescript';

...but is only declared as a dev dependency here:

@typescript-eslint/types/package.json

  "devDependencies": {
    "typescript": "*"
  }

This makes typescript into a phantom dependency, which is an incorrect practice when using a package manager with a modern node_modules layout.

Suggested fix

1. Add typescript to the peerDependencies section for @typescript-eslint/types (maybe as an optional peer dependency if we are lazy), OR
2. Rework ast-spec.d.ts so that it does not rely on imports from typescript

It's possible to work around this problem using hacks, but those hacks would need to be applied to each project in this situation that somehow indirectly depends on @typescript-eslint/types.

Versions

package	version
@typescript-eslint/types	4.28.2
TypeScript	3.9.x and 4.3.x
ESLint	7.30.0
node	12.20.1

[bradzacher]

Note that the dependency is specifically a type-only import - it is not a runtime dependency.

typescript-eslint/packages/ast-spec/src/token/PunctuatorToken/PunctuatorTokenToText.ts

Line 1 in 407a374

import type { SyntaxKind } from 'typescript';

We purposely do not declare a peer dependencies on typescript in any of our packages because there are many packages that depend on our tooling in non-typescript contexts. If we declared a peer dep then yarn/npm/etc would throw warnings at users for no real reason.

We declare the dependency as an optional peer dependency, but it looks like adding it as an optional peer dependency got missed (this comment got missed). We can definitely add the optional peer dependency config like we have for our other packages though!

We could probably hack around our reliance on the latest version using a similar trick to this:

typescript-eslint/packages/typescript-estree/src/ts-estree/ts-nodes.ts

Lines 3 to 10 in 407a374

	// Workaround to support new TS version features for consumers on old TS versions
	// Eg: https://github.com/typescript-eslint/typescript-eslint/issues/2388, https://github.com/typescript-eslint/typescript-eslint/issues/2784
	declare module 'typescript' {
	/* eslint-disable @typescript-eslint/no-empty-interface */
	export interface NamedTupleMember extends ts.Node {}
	export interface TemplateLiteralTypeNode extends ts.Node {}
	/* eslint-enable @typescript-eslint/no-empty-interface */
	}

[octogonz]

Note that the dependency is specifically a type-only import - it is not a runtime dependency.

This would be the right criteria if the .d.ts file was not part of the exported public API. But it is.

We can definitely add the optional peer dependency config like we have for our other packages though!

We could probably hack around our reliance on the latest version using a similar trick to this

Both sound good to me. I can make a PR, but let me know if you prefer a particular approach.

[bradzacher]

definitely the optional peer dep so we're explicit (I know yarn2 can break sometimes without this):

typescript-eslint/packages/typescript-estree/package.json

Lines 69 to 73 in b15a2b2

	"peerDependenciesMeta": {
	"typescript": {
	"optional": true
	}
	},

As for some form of "manually define the new enum members" hack so we can ensure we're backwards compatible.... it looks like we can't do it :(

import { SyntaxKind } from 'typescript';

declare module 'typescript' {
  export enum SyntaxKind {
    BarBarEqualsToken = -1,             // Duplicate identifier 'BarBarEqualsToken'.
    AmpersandAmpersandEqualsToken = -2, // Duplicate identifier 'AmpersandAmpersandEqualsToken'.
    QuestionQuestionEqualsToken = -3,   // Duplicate identifier 'QuestionQuestionEqualsToken'.
  }
}

Hmm....
So we don't even actually use the interface anywhere public:

typescript-eslint/packages/ast-spec/src/token/PunctuatorToken/spec.ts

Lines 1 to 12 in 407a374

	import type { AST_TOKEN_TYPES } from '../../ast-token-types';
	import type { BaseToken } from '../../base/BaseToken';
	import type { PunctuatorTokenToText } from './PunctuatorTokenToText';
	export * from './PunctuatorTokenToText';
	type ValueOf<T> = T[keyof T];
	export interface PunctuatorToken extends BaseToken {
	type: AST_TOKEN_TYPES.Punctuator;
	value: ValueOf<PunctuatorTokenToText>;
	}

The interface exists so that we can ensure we're declaring all of the token strings based on what TS declares (this set of enum members here: https://github.com/microsoft/TypeScript/blob/8e795108eb0d35b36048ee1b0e55ca154a72a5ea/src/compiler/types.ts#L481-L540)

So externally the interface's keys aren't seen, just the interface's values (which are not based on TS)

But API extractor can't automatically eliminate this complex relationship for us. I wonder if there's a way to hack this in to the extractor step?

[octogonz]

I feel like there is a simpler solution. Let me take a crack at it over the weekend.

[octogonz]

Draft PR #3623 illustrates how the test could be rewritten so that the mapping table does not become part of the public API.

It requires maintaining two lists (PunctuatorTokenValue and the test table). However the test is improved to be bidirectional, so it will now also fail if PunctuatorTokenValue is has extra members. Thus although we have to add a new entry in two places, mistakes should be impossible.

With this change, import type { SyntaxKind } from 'typescript'; no longer appears in the .d.ts rollup.

Let me know if you like this solution.

[octogonz]

BTW I was curious about how this project gets built:

api-extractor.json reads <projectFolder>/dist/index.d.ts and writes <projectFolder>/dist/ast-spec.ts which has a .ts file extension instead of the normal .d.ts. Then copy-ast-spec.ts rewrites the content to be a .ts file, so it can get compiled to a .d.ts file again. Why was that necessary? .d.ts files can be used as compiler inputs.

[bradzacher]

Why was that necessary?

It's not a .d.ts file because it contains enum declarations which we need exposed at runtime.

[MichaelDeBoey]

Thus although we have to add a new entry in two places

The whole point of doing it like I did, was to not have to duplicate values and keeping them in sync.

Although #3529 created the tests, so it's now checked that every value in the TS SyntaxKind is present, I think there's less harm in duplicating it.

so it will now also fail if PunctuatorTokenValue has extra members

This makes your PR is removing 3 values that are currently present though 🤔

[octogonz]

I think there's less harm in duplicating it.

Besides eliminating the typescript dependency, the new code is simpler (i.e. easier to read) which probably outweighs the reduced duplication (i.e. a bit less work for people who add new definitions). Reading>Writing

so it will now also fail if PunctuatorTokenValue has extra members

This makes your PR is removing 3 values that are currently present though 🤔

I was curious about that. The current code references definitions that don't exist in the typescript version that's being used to build it, and thus are not getting validated. Was that intentional?

Since typescript is devDependencies only, why wouldn't we simply upgrade the compiler if we want to support that?

[MichaelDeBoey]

Was that intentional?

These 3 values were already in there before I did the refactor, so that's why I kept them

why wouldn't we simply upgrade the compiler if we want to support that?

They will be included in v4.4 (hence the comments next to these values)

[octogonz]

@MichaelDeBoey I see -- v4.4 hasn't been released yet.

1. If you think someone would realistically depend on them, then I can temporarily include them.
2. If you think that generally speaking typescript-eslint should facilitate usage of unpublished compiler features, I can reduce the validation so that it ignores extra members. That seems... a bit of a hassle to support, particularly if nobody explicitly requested it.
3. Otherwise we could simply make it more strict and focus on supporting official TypeScript.

Let me know which way you want to go. And if there are no objections to my approach otherwise, I can finish the draft PR and get this wrapped up.

[MichaelDeBoey]

@octogonz

1. If you think someone would realistically depend on them, then I can temporarily include them.

It's currently out there, so I think we should keep supporting them and include them temporarily.
The "problem" here is that v4.3 should have included them already imo, but didn't.

2. If you think that generally speaking typescript-eslint should facilitate usage of unpublished compiler features, I can reduce the validation so that it ignores extra members. That seems... a bit of a hassle to support, particularly if nobody explicitly requested it.

I think we should only support released features, so having a temporary solution would fit for now I think.

3. Otherwise we could simply make it more strict and focus on supporting official TypeScript.

We can do this after v4.4 is released and set as the dependency in this package I think.

As I'm not a maintainer on this project, let's see what @bradzacher thinks.

[MichaelDeBoey]

If we decide to go the new way @octogonz did in #3623, I'll need to change the way I implemented #3570 btw

[octogonz]

I'm still waiting to hear from @bradzacher

[MichaelDeBoey]

Maybe @JamesHenry can also help out here?

[bradzacher]

Sorry to delay. I've been taking some personal time off of open source for the last few weeks (trying to avoid anything non-trivial). I'll get back to this next week. I've got quite a backlog to traverse

[elvorad]

I am upvoting and watching this issue because it currently breaks compatibility to TypeScript 3.9.9 for me. According to the readme, typescript-eslint is backward-compatible until TypeScript 3.3.1. typescript-eslint expects to find BarBarEqualsToken, AmpersandAmpersandEqualsToken and QuestionQuestionEqualsToken but cannot find it in the TypeScript 3.9.9 ast-specs (they were added in TypeScript 4.0.0).
I would be happy to see an upgrade. Until then, I will stay at typescript-eslint@4.26.0 which works fine in my case.

[bradzacher]

Oookay.

First up - in regards to the build process.. Yeah it is pretty hacky.

In a nutshell: When I split the AST spec into its own "package", I had two goals:

1. I wanted it to be a "private" package - I didn't want to publish it to npm because nobody should ever need the types without the parser, and I didn't want to introduce a useless package to confuse people.
2. I needed it to be a non-breaking change as I didn't want to go through a major version bump to merge the PR.

So my solution was to use a type bundler to flatten the spec into a single file and then copy it across to types. This worked as it essentially made the change a no-op (I essentially replaced the manually maintained file with an almost identical one generated by the build process).

I had to hack it a bit because every bundler made the enums a declaration.. so I had to make them non-declared when I copied it across to the source directory.

typescript-eslint/packages/types/tools/copy-ast-spec.ts

Line 54 in 15f7184

code.replace(/export declare enum/g, 'export enum'),

I know you've worked on this stuff before @octogonz, so if you've suggestions for a better way to do it I'd be happy to take suggestions (or a PR 😄 ).

---

If you think that generally speaking typescript-eslint should facilitate usage of unpublished compiler features

I think we should only support released features

My take is that we should support whatever the types exported by TS supports to keep maintenance as simple as possible. I'd rather have a few "unreleased" members and strict consistency than exclude them and have the possibility of us forgetting to add new members later.

---

I do agree that I like not having to duplicate things.

Thinking out loud... I wonder if there is a way we can just accomplish this via a build step?
I.e. use the TS API to get the type of ValueOf<PunctuatorTokenToText>, then print it, then replace ValueOf<PunctuatorTokenToText> with the printed value?

Probably way more work than we should put in though TBH.

---

@octogonz might know... can we get around this by using the bundledPackages flag for API extractor? Would that inline the SyntaxKind declaration?

Seems like it might just be another hacky hack in the system. Might be good to just "do it properly"..

Ultimately I'm okay with duplicating it. It's probably the easiest way for us to remove the unnecessary dependency.
We'll have to ensure we don't export PunctuatorTokenValueTestMap from the root index.ts so that it doesn't get bundled - which is easy enough.

[bradzacher]

Interesting - I tried quickly adding bundledPackages: ['typescript'] to the config, and it crashed api-extractor:

ERROR: Internal Error: Unable to analyze the export "SyntaxKind" in
/Users/bradzacher/git/typescript-eslint/node_modules/typescript/lib/typescript.d.ts
You have encountered a software defect. Please consider reporting the issue to the maintainers of this application.

[octogonz]

@octogonz might know... can we get around this by using the bundledPackages flag for API extractor? Would that inline the SyntaxKind declaration?

It would, but because the TypeScript API is complex and not factored nicely for API Extractor, it is likely to pull in unrelated definitions.

Before we start setting up a repro and debugging API Extractor, I'd like to understand what is wrong with the draft PR #3623. What problem are we trying to solve? As I understand it:

Pros for PR #3623 approach:

• The approach is straightforward and easy to understand, because it doesn't rely on a custom build tool or Ph.D. type algebra
• The approach is easy to maintain
• The approach makes it possible to pull in definitions from a future/past TypeScript version if needed, even though you said generally you want to avoid that

Cons:

• A bit of manual work is required to update the list whenever we upgrade the TypeScript compiler. However these manual updates are not error-prone, because the correspondence is validated during the build.

A complex inlining transform could eliminate this manual work, but potentially with the cost of making the overall system harder for a casual observer to understand. That might not even be a compelling tradeoff. I guess I'm just questioning why we should invest more time in this problem if we already have an okay solution.

[bradzacher]

I was just wondering if we could hackily solve this with a one-line change. But the bug prevents it. 🤷
I'm fine with cutting that line of thinking there.

For sure - I'm okay with the duplicating route as long as the tests ensure we don't accidentally desync.

[MichaelDeBoey]

As long as we can keep all values that are currently present, I'm OK with duplicating (until we find a better solution) too 👍

[octogonz]

Ok, lemme find some time, and I will fix up my draft PR so we can get this fixed.

[MichaelDeBoey]

@octogonz Is there something I can help out with to get your PR ready?

[octogonz]

IIRC the build was failing, maybe because of a unit test. If someone can fix that, it would be helpful. I've been on vacation so haven't had time to look at it yet.

[MichaelDeBoey]

@octogonz I've been taking a look at this and it seems the errors are coming from the fact that TokenToText is extending my extracted PunctuatorTokenToText and now isn't available anymore.

typescript-eslint/packages/typescript-estree/src/node-utils.ts

Lines 16 to 24 in 043462e

	interface TokenToText extends TSESTree.PunctuatorTokenToText {
	[SyntaxKind.ImportKeyword]: 'import';
	[SyntaxKind.InKeyword]: 'in';
	[SyntaxKind.InstanceOfKeyword]: 'instanceof';
	[SyntaxKind.NewKeyword]: 'new';
	[SyntaxKind.KeyOfKeyword]: 'keyof';
	[SyntaxKind.ReadonlyKeyword]: 'readonly';
	[SyntaxKind.UniqueKeyword]: 'unique';
	}

I could expose your PunctuatorTokenValueTestMap, but then we would be in the same problems as we currently are I think, since we expose an object that has the TS SyntaxKind in it? 🤔

[octogonz]

Thanks, that's helpful! It seems that when I made my draft PR, I didn't try building the entire monorepo, so I wasn't aware that @typescript-eslint/typescript-estree internally relies on the TSESTree.PunctuatorTokenToText definition. The extended TokenToText gets used like this:

typescript-eslint/packages/typescript-estree/src/node-utils.ts

Lines 56 to 62 in 62bcc93

	export function getTextForTokenKind<T extends ts.SyntaxKind>(
	kind: T,
	): T extends keyof TokenToText ? TokenToText[T] : string | undefined {
	return ts.tokenToString(kind) as T extends keyof TokenToText
	? TokenToText[T]
	: string | undefined;
	}

This wrapper maps the ts.SyntaxKind types to their corresponding string token, which seems to be part of an elaborate compile-time validation of the visitor that converts a tree of TypeScript objects to a tree of ESLint objects.

For example:

typescript-eslint/packages/typescript-estree/src/convert.ts

Lines 2375 to 2381 in 62bcc93

	if (node.readonlyToken) {
	if (node.readonlyToken.kind === SyntaxKind.ReadonlyKeyword) {
	result.readonly = true;
	} else {
	result.readonly = getTextForTokenKind(node.readonlyToken.kind);
	}
	}

In the above code node.readonlyToken.kind has type ts.SyntaxKind.PlusToken | ts.SyntaxKind.MinusToken, whereas result.readonly has type '+' | '-'. The TypeScript compiler API implements the runtime conversion in ts.tokenToString(), but it does not expose this mapping in the type system. Thus PunctuatorTokenToText has to be manually declared in @typescript-eslint/typescript-types and then gets imported and extended in @typescript-eslint/typescript-estree.

Thinking about the big picture:

1. typescript-eslint originated from the idea that the compiler's APIs are complex and brittle across upgrades, because the AST is mixed together with advanced APIs for semantics and transforms
2. By contrast, most ESLint rules do relatively simple syntax checks, that need just an AST, and essentially just the JSON-serializable tree without all the business logic
3. Thus the big innovation of @typescript-eslint/types was to replace the compiler's ts.Node tree with an ESLint-specific TSESTree.Node tree that isn't tangled up with typescript.d.ts
4. However the internal code that constructs the TSESTree.Node tree is complicated, so it proved useful to craft elaborate type algebra that makes sure the two trees match up.
5. But in doing so, we inadvertently made @typescript-eslint/types dependent on typescript.d.ts. This accident was hidden behind the phantom dependency identified in @typescript-eslint/types has a phantom dependency on the "typescript" package #3622
6. Although @typescript-eslint/types depending on typescript.d.ts made the code base easier to maintain, it causes a bunch of trouble for consumers of the NPM package, for the exact same reasons from #1 above.

Some options:

• (Generally I would tend towards dispensing with getTextForTokenKind() and instead using ts.tokenToString() with simple string | undefined types. Sometimes there's advantages to keeping things simple rather than trying to ensure that every possible mistake gets caught as compile error. That doesn't seem to fit the culture of this code base, though.)
• We could go back to the idea of an optional peer dependency. But since typescript.d.ts is part of the API, the dependency would need to be on the latest version, which is going to cause annoying side-by-side versions for an NPM package that is 57.9MB unzipped, and already a nontrivial part of npm install download times.
• Another idea would be to extract the PunctuatorTokenToText stuff into an "internal API" or utility package. It looks like PunctuatorTokenToText is ONLY over used internally to build the @typescript-eslint repo -- if so, then we should be able to hack this together pretty easily.

The last one seems like a good avenue to explore.

[MichaelDeBoey]

The TypeScript compiler API implements the runtime conversion in ts.tokenToString(), but it does not expose this mapping in the type system.

So if we can get these types exposed by TypeScript, we could remove the whole TokenToText map and could just go for the solution you're currently having, right? 🤔

Generally I would tend towards dispensing with getTextForTokenKind() and instead using ts.tokenToString() with simple string | undefined types. Sometimes there's advantages to keeping things simple rather than trying to ensure that every possible mistake gets caught as compile error. That doesn't seem to fit the culture of this code base, though.

Just using ts.tokenToString() would remove some type-safety and has the implication that we need to do extra checks/casts in the rest of the codebase (as TS wouldn't know the runtime value), which we get for free with the type-safety.

That's actually where the whole idea of extracting PunctuatorTokenToText came from: make PunctuatorToken's value type not just string, but specify it so we have more type-safety and we can simplify some other types by creating generics for them (see #3496 & #3461 (comment))

Another idea would be to extract the PunctuatorTokenToText stuff into an "internal API" or utility package. It looks like PunctuatorTokenToText is ONLY over used internally to build the @typescript-eslint repo -- if so, then we should be able to hack this together pretty easily.

Wouldn't that cause the same problem as we have right now?
@typescript-eslint/types is already an internal package.
So having another internal package where @typescript-eslint/types would depend on, is just moving the exact same problem to that other package I think. 🤔

[octogonz]

So if we can get these types exposed by TypeScript, we could remove the whole TokenToText map and could just go for the solution you're currently having, right? 🤔

That's a solution I hadn't thought of. However these requirements /might/ be somewhat specific to typescript-eslint. In my experience, most TypeScript analysis tools would simply work with ts.SyntaxKind rather than a 1:1 mapping onto the text strings returned by ts.tokenToString(). TSESTree.Node has this need mainly because it exactly duplicates the TypeScript AST.

Wouldn't that cause the same problem as we have right now? @typescript-eslint/types is already an internal package.

Lemme follow up on this question. (IIRC the whole problem I had is that @typescript-eslint/types is not "internal", it was getting compiled by my lint rule project, but I need to double-check that.)

[octogonz]

Yes, when my project imports TSESTree like this...

import type { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils';

...it imports from @typescript-eslint\types\dist\ts-estree.d.ts which imports @typescript-eslint\types\dist\ast-spec.d.ts. That's what failed to compile because my typescript.d.ts wasn't the latest version (due to the phantom dependency).

So @typescript-eslint/types is definitely not an "internal" API for someone who is authoring lint rules using @typescript-eslint/experimental-utils.

This whole problem seems to boil down to a conflict that:

• TSESTree is currently defined using imports from typescript.d.ts, which feels wrong because...
• TSESTree is meant to completely decouple the lint rules from the TypeScript API

We can preserve PunctuatorTokenToText by converting it into an internal API (i.e. imported from a special entry point for use in the typescript-eslint repo only). But this means TSESTree.Node cannot be an algebraic transform of ts.SyntaxKind, it needs to be copy+pasted+validated like in my current PR, or else copy+pasted automatically by a preprocessor like copy-ast-spec.ts.

I'm out of time for today, but I'll see if I can come up with another iteration along these lines. Or if someone else has ideas, that would help push this along.

[MichaelDeBoey]

Yes, when my project imports TSESTree like this...

import type { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils';

...it imports from @typescript-eslint\types\dist\ts-estree.d.ts which imports @typescript-eslint\types\dist\ast-spec.d.ts. That's what failed to compile because my typescript.d.ts wasn't the latest version (due to the phantom dependency).

@octogonz Creating a new @typescript-eslint/whatever-name-you-want package will still have the same problem, no?
As @typescript-eslint\types will then depend on @typescript-eslint/whatever-name-you-want and @typescript-eslint/whatever-name-you-want will depend on TypeScript's SyntaxKind.
I think I'm missing something out. 🤔

[MichaelDeBoey]

@bradzacher What do you think is the best way forward here?

[bradzacher]

Sometimes there's advantages to keeping things simple rather than trying to ensure that every possible mistake gets caught as compile error. That doesn't seem to fit the culture of this code base, though.

If we had a dedicated set of maintainers that were onboarded onto tribal knowledge and able to properly maintain and review everything - I'd be all for that approach.
But given we have one-off contributors, and irregular maintainer schedules - the compile-time checks are important to help ensure things are correct without in-depth code reviews.

Another idea would be to extract the PunctuatorTokenToText stuff into an "internal API" or utility package. It looks like PunctuatorTokenToText is ONLY over used internally to build the @typescript-eslint repo -- if so, then we should be able to hack this together pretty easily.

This is the crux of the issue - we have these interfaces that are really internal to the codebase, but are exposed via the types.

Eg we have PunctuatorTokenToText exposed in the types like this:

 export interface PunctuatorToken extends BaseToken { 
   type: AST_TOKEN_TYPES.Punctuator; 
   value: ValueOf<PunctuatorTokenToText>; 
 } 

However PunctuatorTokenToText isn't ever actually directly consumed by any public types - so really it shouldn't be exposed publicly.

This is a little bit difficult to solve because right now:

1. we have these types declared in /types, but consumed from /typescript-estree, so they are "public" from the /types package.
2. we currently do lazy type declaration production with no bundling - meaning all of our "internal" functions are exposed publicly from the types.
3. api-extractor does not inline the TS enum.
4. api-extractor does not resolve the complex type to its trivially resolved string union.

In terms of solutions to these problems:

1. This would be "fixable" by scoping the type to be package internal. However there's no good mechanism I know for doing this within the monorepo design whilst also preventing them from leaking externally. I can think of a few potential solutions

   1. build two sets of type declarations - an internal and external version, and then use package.json files to ensure we don't include the internal types in the package.
      • I don't know how this would work with typescript's automatic resolution of types based on package.json - it'd probably break it.
   2. do the above, but pick which ones to build based off of whether or not we're doing a bundle build.
   3. copy the relevant files across to /typescript-estree as part of the prebuild step.
      • This wouldn't fix the typescript dependency problem - but it would at least scope the problem entirely to the /typescript-estree package (and it can then be eliminated by (2) )
   4. Build on top of fix(types): Eliminate phantom dependency on "typescript" package #3623 to remove the AST dependency on the enum (removing the dependency from /types), and then make /typescript-estree have a dev-only, type-only import from /ast-spec and deep import the relevant interfaces.
      • This would also need (2) to be complete

2. is trivially solvable by removing truly internal things from the exported type declarations. We can do this by either:

   1. annotating internal things with @internal and turning on the TS compiler option stripInternal
   2. introducing api-extractor into /typescript-estree to bundle the types based on what's actually exported from the package.

3. is hard blocked by the aforementioned bug in api-extractor.

4. is hard to fix because whilst we humans can see this type is trivially resolvable, there's obviously no way to teach api-extractor that we want this. We could conceivably add a post-compilation step (similar to the enum hack transform that's there) which does this inlining for us, but it's kind of hacky.

In terms of picking a solution:

(3) would be the "simplest" solution from the POV of our codebase as it would be a one-line change and everything would "just work", but it's probably a huge pain in the ass to find and fix this bug in api-extractor (if it's even possible).

(4) would only half solve the problem because /typescript-estree would still have the dependency on the TokenToText type. So it'd only work in tandem with (2). It's also not hugely desirable because it's a hacky hack and would be hard to code up.

---

I think the best path forward might be something like (1.iv) + (2.ii).

To clarify, what I'd suggest is this:

1. Build on top of fix(types): Eliminate phantom dependency on "typescript" package #3623 to allow us to change the AST spec so that it never ever directly imports the interfaces that include the TS enum.
   • This will fix the implicit /types dependency.
2. Deep import from /ast-spec from /typescript-estree to get at the desired interfaces
3. Use api-extractor to setup a declaration file which does not expose the internal functions that touch the interfaces.
   • This will fix the implicit /typescript-estree dependency.

What do you guys think?

[MichaelDeBoey]

@octogonz Any news on the new implementation?

[octogonz]

To clarify, what I'd suggest is this:

1. Build on top of fix(types): Eliminate phantom dependency on "typescript" package #3623 to allow us to change the AST spec so that it never ever directly imports the interfaces that include the TS enum.

   • This will fix the implicit /types dependency.

2. Deep import from /ast-spec from /typescript-estree to get at the desired interfaces

3. Use api-extractor to setup a declaration file which does not expose the internal functions that touch the interfaces.

   • This will fix the implicit /typescript-estree dependency.

This sounds good, although I'm a bit fuzzy on the specific details to implement it. (I've forgotten some of the context and haven't had time to revisit this properly.)

My main feedback is that this problem has been unfixed for a while... so if there's a quick+dirty hack to unblock people, maybe we could do that first, and then follow up with the better long-term solution.

2. is trivially solvable by removing truly internal things from the exported type declarations. We can do this by either:
   i. annotating internal things with @internal and turning on the TS compiler option stripInternal

@bradzacher Not sure it's relevant, but FYI you could also use API Extractor to trim @internal definitions, and it can generate two different .d.ts rollups (an "internal" one and an "public" one). This way, your monorepo projects can access each other's internals, while preventing public consumers from doing that.

Any news on the new implementation?

Unfortunately I've become busy with other things lately, and haven't had time to work on this. 🙁 If I do get some time, I'll post an update on this thread.

[MichaelDeBoey]

@octogonz I was wondering if you already had some time to look further into this?

[stagas]

There are more dependencies on typescript that are not being listed:

https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/src/util/astUtils.ts#L3

https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/package.json#L46-L77

The package is using only its types, however the CJS code that is generated is doing a require():

Object.defineProperty(exports, "__esModule", { value: true });
exports.forEachReturnStatement = exports.getNameLocationInGlobalDirectiveComment = void 0;
const escapeRegExp_1 = require("./escapeRegExp");
const ts = __importStar(require("typescript")); // <------------------here
// deeply re-export, for convenience
__exportStar(require("@typescript-eslint/utils/dist/ast-utils"), exports);

and this breaks various tools because they're not pulling in that dependency.

[bradzacher]

eslint-plugin has an explicitly listed optional peer dependency:

typescript-eslint/packages/eslint-plugin/package.json

Lines 73 to 77 in 7735475

	"peerDependenciesMeta": {
	"typescript": {
	"optional": true
	}
	},

Sometimes people install these packages as transitive dependencies. If we list an explicit peer dependency we would list typescript: "*", which would trigger warning messages to anyone who has had this package installed transitively and does not use it. This happens a lot in the ecosystem as people setup general purpose linting packages which support both TS and non-TS usecases.

The optional dependency config above is is supported by yarn2, npm and pnpm. It causes them to understand that our package depends on TS.

Why a peer dep and not a proper dep? Because we want to use the version of TS installed locally, and never our own.

The package is using only its types

This is false. That very file uses the TS export at the bottom of the file:

typescript-eslint/packages/eslint-plugin/src/util/astUtils.ts

Lines 57 to 73 in 7735475

	case ts.SyntaxKind.ReturnStatement:
	return visitor(<ts.ReturnStatement>node);
	case ts.SyntaxKind.CaseBlock:
	case ts.SyntaxKind.Block:
	case ts.SyntaxKind.IfStatement:
	case ts.SyntaxKind.DoStatement:
	case ts.SyntaxKind.WhileStatement:
	case ts.SyntaxKind.ForStatement:
	case ts.SyntaxKind.ForInStatement:
	case ts.SyntaxKind.ForOfStatement:
	case ts.SyntaxKind.WithStatement:
	case ts.SyntaxKind.SwitchStatement:
	case ts.SyntaxKind.CaseClause:
	case ts.SyntaxKind.DefaultClause:
	case ts.SyntaxKind.LabeledStatement:
	case ts.SyntaxKind.TryStatement:
	case ts.SyntaxKind.CatchClause:

[stagas]

@bradzacher Thank you. Yes you are right, it was my mistake, I didn't read very carefully and i confused those with being types.

It's just odd that a package requires a dependency that is not listed in dependencies or at least peerDependencies. That meta field implies it's an optional dependency, but why is it optional since it's definitely required? I think it should be in peerDependencies instead?

[bradzacher]

From my last comment

Sometimes people install these packages as transitive dependencies. If we list an explicit peer dependency we would list typescript: "*", which would trigger warning messages to anyone who has had this package installed transitively and does not use it. This happens a lot in the ecosystem as people setup general purpose linting packages which support both TS and non-TS usecases.

If someone doesn't install TS and uses our rules it'll crash. That's on them.
For everyone else we'll use the locally installed version of TS as required by the optional metadata.

[stagas]

Ok it's super weird this behavior, but i understand now. Thanks for clarifying twice, sorry about my confusion :)