chore: add typescript as optionalDependency #1046
Conversation
|
Thanks for the PR, @kaicataldo! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
| @@ -16,6 +16,8 @@ | |||
| npm install @typescript-eslint/parser --save-dev | |||
| ``` | |||
|
|
|||
| Note: `@typescript-eslint/parser` assumes you have `typescript` already installed. Please read the documentation [here](https://github.com/typescript-eslint/typescript-eslint#supported-typescript-version) for more details. | |||
There was a problem hiding this comment.
probably want this in eslint-plugin as well, as eslint-plugin also directly consumes typescript.
| @@ -61,5 +61,8 @@ | |||
| "glob": "^7.1.4", | |||
| "lodash.isplainobject": "4.0.6", | |||
| "typescript": "*" | |||
| }, | |||
| "optionalDependencies": { | |||
There was a problem hiding this comment.
ditto for needing this in eslint-plugin - might as well be consistent :)
parser consumes typescript-estree so it isn't needed there.
| @@ -220,7 +220,7 @@ We will always endeavor to support the latest stable version of TypeScript. Some | |||
|
|
|||
| **The version range of TypeScript currently supported by this parser is `>=3.2.1 <3.7.0`.** | |||
|
|
|||
| This is reflected in the `devDependency` requirement within the package.json file, and it is what the tests will be run against. We have an open `peerDependency` requirement in order to allow for experimentation on newer/beta versions of TypeScript. | |||
| This is reflected in the `devDependency` requirement within the package.json file, and it is what the tests will be run against. We have an open `optionalDependency` requirement in order to allow for experimentation on newer/beta versions of TypeScript. | |||
There was a problem hiding this comment.
maybe would be good to add a comment stating explicitly that we expect the user to install typescript in the project root?
might as well be explicit.
|
|
|
I didn't realise In this case, we'd rather have a peer dependency with {
"peerDependencies": {
"typescript": "*"
},
"peerDependenciesMeta": {
"typescript": {
"optional": true
}
}
}Yarn's had this supported for a bit, pnpm has support for it, and npm added support about a month ago. |
|
I'm going to close this infavour of #990, which I missed earlier, and is doing the optional peer deps config. |
|
Sounds good! |
While debugging eslint/eslint#12376, I was kindly directed by @JamesHenry to #828 (comment). Thought this could maybe made clearer with a README change and adding
typescriptas an optional dependency. Testing with npm<6.11, it seems theoptionalDependencieskey is ignored.Happy to remove the optional dependency addition if the team feels just having the documentation is a better fix for now.