@typescript-eslint/typescript-estree is missing a peer dependency on the "typescript" package

[octogonz]

The package.json for the @typescript-eslint/typescript-estree current canary release looks like this:

{
  "name": "@typescript-eslint/typescript-estree",
  "version": "2.0.0-alpha.4",
  "dependencies": {
    "lodash.unescape": "4.0.1",
    "semver": "^6.2.0"
  },
  "devDependencies": {
    "@babel/code-frame": "7.0.0",
    "@babel/parser": "7.3.2",
    "@babel/types": "^7.3.2",
    "@types/babel-code-frame": "^6.20.1",
    "@types/glob": "^7.1.1",
    "@types/lodash.isplainobject": "^4.0.4",
    "@types/lodash.unescape": "^4.0.4",
    "@types/semver": "^6.0.1",
    "@typescript-eslint/shared-fixtures": "2.0.0-alpha.4",
    "babel-code-frame": "^6.26.0",
    "glob": "^7.1.4",
    "lodash.isplainobject": "4.0.6",
    "typescript": "*"  // <=====
  },
  . . .
}

The typescript dependency appears in the devDependencies section, but it is not listed as a peerDependency. This is incorrect because this package definitely does import objects from the typescript package. For example, this line:

typescript-eslint/packages/typescript-estree/src/tsconfig-parser.ts

Line 189 in 3c902a1

const programWatch = ts.createWatchProgram(watchCompilerHost);

When using NPM and classic Yarn, this "phantom dependency" can get inadvertently satisfied by other packages that appear in the flattened node_modules tree (albeit without any guarantee of what version will get loaded). But with more rigorous package managers such as PNPM or Yarn Plug 'n Play, the module may fail to load because their installation strategy prevents undeclared dependencies from being accidentally imported (while still being 100% compatible with the NodeJS module resolution contract).

Proposed solution: Add something like this to typescript-estree/package.json:

  "peerDependencies": {
    "typescript": ">= 3.0.0"
  }

[octogonz]

There's also a similar problem in @typescript-eslint/parser.

I created PR #829 to fix this.

[bradzacher]

The missing typescript peer dependency is done on purpose.
This is to help with adoption through multi-purpose tools such as create-react-app.

Essentially create-react-app enables users to setup the app as typescript, and manages their linting experience for them - thus it has a permanent dependency on our packages (regardless of the user's language choices).
However being a big package, typescript is only installed by the tooling at the root app level when the user chooses to use it.

If we have a peer dependency on typescript, npm/yarn will spit out warnings into the console whenever users do any operation on the package.json. This creates a bad experience and annoys users, and colours people's perceptions of the language.

Ultimately the peer dep doesn't do anything except provide documentation that the packages use typescript, which is similarly accomplished by the package names, and also the fact that they'll throw at lint-time. But if someone intentionally installs any of our packages, they'll have typescript installed locally anyway.

IIRC yarn supports the notion of optional peer dependencies, but NPM doesn't. The plan is to add the deps back in using that notation when both tools support it.
As I understand it there can be some weirdness with yarn pnp that would be fixed by this. In some specific and rare package configurations (which is currently solved by installing typescript at the root level).

[octogonz]

I need to do a little more investigation to see how likely my project is to get burned by this in practice. Maybe it's a purely theoretical concern. I'm being paranoid because in a large company, people can spend a /lot/ of time dealing with headaches caused by packages with incorrectly specified package.json dependencies.

Ultimately the peer dep doesn't do anything except provide documentation that the packages use typescript

Ermmm no. The peer dependency alters the package manager's analysis, and in some situations this is the only way to get a correct installation. Consider this hypothetical case:

• compiler-stuff-2 depends on typescript@2 and @typescript-eslint/parser@latest
• compiler-stuff-3 depends on typescript@3 and @typescript-eslint/parser@latest
• compatibility-tester depends on both compiler-stuff-2 and compiler-stuff-3 -- let's say it runs both TypeScript 2 and 3 on an input to validate that it's compatible with both

(This example is perhaps a bit artificial, since it's just the first thing I could think of. But it represents a general NPM side-by-side versioning problem that arises very frequently for large scale projects.)

In the above example, when @typescript-eslint/parser calls require("typescript"), should it get TypeScript 2 or 3? The old-fashioned NPM answer is "meh, I dunno, just take the first version you find when probing around in node_modules, and if it's completely missing, then accuse the package manager of being weird and, um, maybe install it globally?" But while this dismissive attitude works fine for people who only deal with simple situations, it cannot possibly give us a correct solution for the sort of problem shown above. It's important to think about dependencies clearly.

The right answer is that we need to load two instances of @typescript-eslint/parser, one bound to TypeScript 2 and one bound to TypeScript 3. But that can only happen if there is a declared dependency or peerDependency to inform the package manager about it.

There are two modern implementations of this feature:

• For PNPM, the peer dependency instructs it to install two folders with @typescript-eslint/parser, one for each peer relationship. Each installation goes in a tricky symlinked folder that is cleverly designed to be 100% compatible with the legacy NodeJS module resolution algorithm. The symlinks ensure that require("typescript") will find the intended version of TypeScript when NodeJS goes probing upwards in fs.realpath parent folders. It perfectly solves this problem.

  Whereas if the peer dependency is missing, then require("typescript") may not find any version at all, because package.json lied about not needing typescript, so the symlink didn't get created.

• For Yarn PNP, (from my sketchy understanding having not used it myself) instead of relying on symlinks, they hook into the NodeJS module resolver and redirect require("typescript") at runtime so it finds the right instance. Thus the effect is the same as PNPM, just without creating a bunch of symlinks.

As I understand it there can be some weirdness with yarn pnp that would be fixed by this.

The "weirdness" is that it correctly supports side-by-side versioning relationships, which requires those dependencies to be actually declared in package.json. From this perspective, classic NPM was more "weird" by flattening the node_modules folder and allowing packages to import undeclared dependencies, with no idea what version they will get.

Essentially create-react-app enables users to setup the app as typescript, and manages their linting experience for them - thus it has a permanent dependency on our packages (regardless of the user's language choices).
However being a big package, typescript is only installed by the tooling at the root app level when the user chooses to use it.

If create-react-app installs TypeScript on demand, then couldn't it use the same approach for other packages that have a peer dependency on TypeScript?

[bradzacher]

Ultimately the peer dep doesn't do anything except provide documentation that the packages use typescript

Sorry I meant in terms of the peer deps we had listed before, which was just typescript: "*". That just tells people to install typescript and provides no useful validation.

(It was probably just part of your example, but) If they use typescript@2, chances are that the plugin will break or work in unexpected ways.
If for some reason there is an old version of typescript used which doesn't support language features (eg pre like 3.5 you could do readonly foo[]), the parse will fail for the file, and eslint will log an explicit lint failure for that file.

Also note that we do runtime checking of the typescript version, and log a big, annoying warning if an unsupported version of TS is being used.

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

Lines 275 to 290 in 42b3013

	function warnAboutTSVersion(): void {
	if (!isRunningSupportedTypeScriptVersion && !warnedAboutTSVersion) {
	const border = '=============';
	const versionWarning = [
	border,
	'WARNING: You are currently running a version of TypeScript which is not officially supported by typescript-estree.',
	'You may find that it works just fine, or you may not.',
	`SUPPORTED TYPESCRIPT VERSIONS: ${SUPPORTED_TYPESCRIPT_VERSIONS}`,
	`YOUR TYPESCRIPT VERSION: ${ACTIVE_TYPESCRIPT_VERSION}`,
	'Please only submit bug reports when using the officially supported version.',
	border,
	];
	extra.log(versionWarning.join('\n\n'));
	warnedAboutTSVersion = true;
	}
	}

These together will likely take care of many of the problems that could happen with different versions.

---

Sorry, weirdness was the wrong word. I should have just said errors.
Yarn pnp throws errors because our packages require packages not listed in the dependencies. The solution is to make sure typescript is installed in the project root, which should be the case anyways.

---

If create-react-app installs TypeScript on demand, then couldn't it use the same approach for other packages that have a peer dependency on TypeScript?

I wasn't actually sure how it worked, as I haven't ever used it before.
Looking at the CRA docs it tells the user to manually install typescript in the project root.

I think they have certain philosophies like not modifying the user's dependencies. The idea is that it's a black-box tool. Linting is handled within the tool (it integrates into their custom webpack build process), hence they have internal dependencies on our packages.

[octogonz]

The solution is to make sure typescript is installed in the project root, which should be the case anyways.

In a monorepo with hundreds of projects, we don't add typescript and tslint and webpack and all that as the devDependencies for every single project. It's too messy to coordinate all these versions, especially when different project kinds need different cocktails of dependencies/versions. Instead, each project depends on a single toolchain package that brings in a standardized rig of packages and configurations. You don't build the project by running tsc directly; instead the npm run build script invokes it via the toolchain.

You are right, though, the project-level dependency on typescript is what makes this work. As soon s I remove that, I suddenly have a much more real-world repro for this problem. 🤔

It's understandable why create-react-app breaks the rules to make life easier for beginners, and why ESLint would want to avoid a bunch of complaints from that audience. Optional peer dependencies sound like a good long term solution. What's the timeframe for NPM support of optional peer dependencies? PNPM already supports it.

[bradzacher]

It looks like it's set to be released in npm v7!

npm/cli#224 (comment)

So give it a few weeks after that for people to start to upgrade their versions, and we can put it in.

[CodeWitchBella]

Its in npm 6.11.0

[SleeplessByte]

I know it's a bit of an abuse of the dependency system, but it's an expectation that the user will have typescript installed as a dependency of their root package when using our tooling, because they themselves are using it to compile their typescript code.

Breaking the eco-system because beginners might not get a great experience when installing packages is not great. The assumption is incorrect, and there is no messaging now when you don't install typescript. In fact, it will work in development, and break as soon as your build your package for production.

I know that this is likely to be fixed (per above), but the reasoning will hopefully not be applied again, in the future.

[bradzacher]

eslint-plugin imports typescript at runtime for every rule that requires type information.
typescript-estree imports typescript at the module import time at runtime to check the installed version.
parser imports typescript-estree at runtime.
The only package that doesn't need typescript at runtime is experimental-utils.

But unless I'm mistaken - a peer dependency wouldn't have helped your prod build.

If your project works in development, that means you have typescript installed in development. Because have it installed as a dev dependency, you wouldn't have gotten a warning logged at install time by npm/yarn.
The only warning you would have seen is if you attempted to do a clean production install via --only=prod, but that probably would have only been done on a CI system, so it would have been missed...

An optional dep will definitely help, because (IIRC) they are installed by default as part of a production install. But it's only been a month since implementation by npm, so I doubt the adoption is there quite yet.

---

I'm curious - which one of our packages are you relying upon in a production environment? Everything we've got is pretty much dev-only tooling.

[SleeplessByte]

Hehe. You're absolutely right. Except I don't agree that typescript is an optional dep. It's required by the main export, so it should be a dep. But alas, like you said, of they're installed by default, that would have fixed it for us 😁🥰.

I use the parser to parse code into an EStree! It's to automatically generate commentary on student solutions for exercises on https://exercism.io. we don't use the typescript parse because using and relying on ESTree means we can actually manually run tools, including custom lint rules.

You can find the repository at https://github.com/exercism/javascript-analyzer

[bradzacher]

The problem with making it a proper dep is that will cause npm/yarn to install a local copy of the package within our install if the version ranges don't match the locally installed version, and having two copies of typescript in your tree can be problematic (imagine having readonly string[] work in your ide, but throw errors at lint time!).

So we kinda have to make it a peer/optional dep :/

[SleeplessByte]

Yessss. I understand. Optional peer dep for me would work. It's a hard problem and the deps system don't provide a right option it seems. Currently it just silently blows up which took two of us an hour to figure out which is costly. But I know you understand :)

…

On 2019-09-25 19:25, Brad Zacher wrote: The problem with making it a proper dep is that will cause npm/yarn to install a local copy of the package within our install if the version ranges don't match the locally installed version, and having two copies of typescript in your tree can be problematic (imagine having readonly string[] work in your ide, but throw errors at lint time!). So we kinda have to make it a peer/optional dep :/ -- You are receiving this because you commented. Reply to this email directly, view it on GitHub [1], or mute the thread [2]. Links: ------ [1] #828?email_source=notifications&email_token=AAO7SWFS5TUUGRTSKBSKTOTQLONHFA5CNFSM4IKROKTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7SWA4I#issuecomment-535126129 [2] https://github.com/notifications/unsubscribe-auth/AAO7SWHJSZ5KHU437WXMETLQLONHFANCNFSM4IKROKTA