Just interesting, why is eslint@^5.0.0 fine but typescript@* not? These packages should not work well if either was lacked. I'm not sure the reason of the unfair.

I left the ESLint peer dep in because that is a hard peer dependency for us, which we do not check at runtime (though nothing will actually work if they don't install it, so it's not really necessary to list it).

On the other hand, we have runtime checks for Typescript in the form of imports (which will hard fail if it's not installed), and also the unsupported version log message.

IIRC - we don't list it as a full dependency so that we don't accidentally create additional versions in the module folder tree.

In regards to CRA - eslint is installed by default by CRA, so that will never warn - but typescript is only installed if it's used. Which means for non-ts users of CRA, they'll continually see the peer dep warning, even though they don't actually need it as a dep.

On the other hand, we have runtime checks for Typescript in the form of imports (which will hard fail if it's not installed), and also the unsupported version log message.

Thank you for elaboration.
I got it if it's true. But, looks like importing typescript statically.

and others.

On the other hand, our packages are used from ESLint, but these don't import eslint package statically except tests. I guess lacking eslint package doesn't throw any runtime error.

Yeah exactly! We have a hard dependency on typescript from within all 3 packages IIRC (eslint-plugin, parser and typescript-eslint).

However we purposely do not list it as a dependency so we don't force users onto a specific version, and so we don't cause two versions to be installed into the tree.

If the end user tries to use any package without typescript installed, every package will hard fail on them.

Considering it was a peerDependency, it is already optional in yarn/npm's eyes, so it means there's no a big difference between listing it and not.

So the only difference between listing it as a peerDependency and not is this: if we list it, yarn/npm will log a warning at install time (which most people ignore anyways).

Ahh, I had misunderstood what you are saying. I understand now. Thank you!

One consequence of this change is that typescript-eslint packages will no longer work with Yarn's plug'n'play installation strategy. Yarn has added the peerDependenciesMeta field to package.json to allow peer dependencies to be optional, so you could do:

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

However, I don't believe NPM respects this field (yet?), so the warning would still be there for NPM users.

@edsrzf - are you sure?

Reading the documentation for pnp:

https://yarnpkg.com/en/docs/pnp/troubleshooting

Or, finally, the jest-environment-jsdom package can be specified as dependency of the top-level package - usually your own. In those circumstances, Yarn will allow Jest to access it even if it isn’t declared in its dependencies (we do this because this is the only case where this is safe: it isn’t possible for the top-level dependencies to be ambiguous).

Ofc it does mention that

The third option is typically meant as a way to unblock yourself, but please report it on the repository of the affected package and link them to this documentation

It's a shame that we can't "properly" support it, but if we specify the peer dep explicitly then npm consumers of tools like react-create-app would still get warnings.
If (/when) NPM implements this mechanism; then we can definitely add it back in.

I confirm what @edsrzf says; this change breaks the support for Yarn PNP :

Error: Cannot read config file: __PATH__/packages/eslint-plugin-swissquote/index.js
Error: Package "@typescript-eslint/typescript-estree@1.8.0" (via "...55b7b3e418d91d1d0226/node_modules/@typescript-eslint/typescript-estree/dist/parser.js") is trying to require the package "typescript" (via "typescript") without it being listed in its dependencies (lodash.unescape, semver, @typescript-eslint/typescript-estree)
Referenced from: /var/folders/f9/2cxw7pps12jg4v4757p565rc0000gn/T/crafty-config-40824q10CkgUvVag5.json

I'm trying to understand the reasoning here. It seems, according to this thread there are two options:

1. Have a package that works in all conditions, but has a one line warning in some cases.
2. Have a package that is broken on purpose to avoid a warning in some cases.

And your conclusion is to break the package for all those who rely on Yarn PNP ?

@edsrzf offered a solution that would display the warning only if the users uses NPM and don't use TypeScript. It's not perfect as it still shows the warning to these users, but at least it doesn't break the package on purpose for others...

Can you please revert this change ?
In the meantime I'll just stay on version 1.7.0

According to the linked yarn pnp docs, if it is listed in the dependencies of your package, then pnp should allow it to work.
If that's not working then either their docs are wrong, or there is a bug in yarn pnp.

edsrzf offered a solution that would display the warning only if the users uses NPM and don't use TypeScript

Like I said before; if (/when) NPM implements this mechanism; then we can definitely add it in and revert this change with it.

In the case of yarn workspaces the top-level might not have a dependency to typescript as in this context the app is most likely a sub-package.

Otherwise, would it be possible to give an instance of TypeScript as an option to the rule that would be passed around ?

couldn't you just -W add it as a root workspace dep then?
if you are running commands from the root workspace instead of the project package, then all dependencies for the lint should be placed up there, or else I assume it'll fail regardless?
I'm not sure on the specifics of eslint here (The docs don't specify anything about this), but I'm pretty certain that if you have a hierarchy of configs, it will require that plugins and plugin deps are installed in the folder (or parent of the folder) that eslint is run from?

For example, the only package in this repo that references the tsutils package is eslint-plugin.
If I ran a script which requires tsutils within the root folder (and not the eslint-plugin folder), then it would fail, because the yarn workspace install doesn't hoist the dependency up.

We could potentially add an option to pass in the pre-required typescript module, though I think it would only ever be useful in this specific case.

In my specific case, I created a shared ESLint configuration that doesn't need the plugins installed in the root package as they are embedded (https://github.com/swissquote/crafty/tree/master/packages/eslint-plugin-swissquote, more specifically : https://github.com/swissquote/crafty/blob/master/packages/eslint-plugin-swissquote/src/utils.js#L1-L17 and https://github.com/swissquote/crafty/blob/master/packages/eslint-plugin-swissquote/src/best-practices.js#L92-L95 )

So that means that when someone installs this shared config/plugin, they get all the dependencies installed with it as it is expected for most npm packages.

There are many tools that accept receiving plugins/ libraries as options, for example webpack's loader configuration, ts-loader's TypeScript configuration, Jest transformers, ESLint's parsers

Wait, why wouldn't "typescript": "*" work? Surely that would prevent double installs?

looking at your repo - why did you create a plugin for it, instead of just a config?
You only need to create a plugin if you're exposing custom rules, but your plugin just exposes configs.
You could just create a config which accomplishes the same thing: https://github.com/bradzacher/eslint-config-brad#readme

The problem with creating a plugin which absorbs the rules like you've done is that people could configure rules twice - once with the real rule name, and once with your namespaced rule name, because those packages are available to npm regardless.

In this case though, people should have typescript installed as a direct dependency of their project, because they will need access to the typescript compiler locally.

In which case "typescript": "*" will help prevent double installs.
Failing that they can use yarn resolutions to ensure only one version gets installed (I do this all the time because some packages hard link against certain versions of types [graphql and react packages are notorious for this], causing duplicate installs).

looking at your repo - why did you create a plugin for it, instead of just a config?

True, I could have made a config, but I currently expose the rules of the other plugins, and the difference is, to my knowledge, only semantic

The problem with creating a plugin which absorbs the rules like you've done is that people could configure rules twice

I'm well aware that this is not an ideal solution, and the eslint-disable statements in the code are very verbose when they are needed. But people don't configure the rule twice as this package is a configuration, which configures the rules for them. And the way it's done the rules are prefixed with the packages' name and the "original" rules aren't available.

However I'm waiting for release of ESLint 6 containing eslint/eslint#11388 discussed in eslint/rfcs#7 to loading plugins and configurations relative to where they are defined. Which removes the need to add the dependency to the plugin in addition to the config

Failing that they can use yarn resolutions to ensure only one version gets installed

Yes it's a very useful feature to modify a dependency, but can only be of used if the dependency is here, it won't help if the dependency isn't specified