chore(eslint-config): mark typescript as a peer dependency #113
Conversation
This isn't _strictly_ necessary, but it is an accurate description of the requirements of the project. We go with the somewhat unsatisfying range of "*" to to minimize the risk of introducing duplicates in a consuming project. At the moment, we're using v4+ in this monorepo, 3.9.7 in liferay-portal, and the lowest range that we're currently using in any other project is "^3.6.3" (in liferay-js-toolkit). In the future, we should be able to converge on v4+. If you have a ".ts" file in your project, you are going to have `typescript` in your `devDependencies`, and our `overrides` configuration is going to use `@typescript-eslint/parser`, which in turn is going to require `typescript`, transitively, in `@typescript-eslint/typescript-estree` here: https://github.com/typescript-eslint/typescript-eslint/blob/c5ffe8833da7875e82953de117b0abe6fa8a60b0/packages/typescript-estree/src/parser.ts#L2 And all will work, even though `@typescript-eslint/typescript-estree` doesn't explicitly declare its hard dependency on `typescript` in its package.json: https://github.com/typescript-eslint/typescript-eslint/blob/c5ffe8833da7875e82953de117b0abe6fa8a60b0/packages/typescript-estree/package.json#L41-L49 But if you don't have any ".ts", then you shouldn't need to have `typescript` in your dependency graph, and we won't use `@typescript/eslint-parser`. So, it makes sense for us to call this an `peerDependency` and mark it as `optional` as per these posts: - https://medium.com/@noamgjacobsonknzi/what-is-peerdependenciesmeta-acea887c32dd - npm/cli#1247 (Note that the `peerDependenciesMeta` field isn't documented yet.) You could also argue that this should be under `optionalDependencies`: https://docs.npmjs.com/files/package.json#optionaldependencies But I think "optional peer" comes closer to the reality here. `optionalDependencies` is for things that you can add, but the package should work if they're not there. That's not the case here. The package won't work if you have ".ts" files but no `typescript`. Related: #91 (comment)
As reported here:
#91 (comment)
Our use of the `@typescript-eslint/parser` (since 50a4d99) means
that we have a runtime dependency on `typescript` package. This worked
fine in liferay-portal, where `typescript` is already present, but not
in liferay-learn, where it is not. And I didn't know that
`@typescript-eslint/parser` doesn't actually declare all of its
transitive dependencies; as noted in:
#113
`@typescript-eslint/typescript-estree` does an unguarded `import` of
`typescript` even though it is not declared in its dependencies (they
declare it as an optional peer dependency).
So, let's not only fix this, by explicitly declaring our dependency on
TypeScript, but also declare all of our TS-related dependencies. This
will allow us to delete the devDependencies currently declared here:
https://github.com/liferay/liferay-portal/blob/f1a2b2ce06fa050d5e1ab193728ab9ec06df4315/modules/apps/remote-app/remote-app-client-js/package.json#L3-L6
|
I'm assuming you know what you're doing, so go ahead 😂 If I understand correctly, this means that you'll always need to have both What does this mean for those simply adding |
Not exactly. If you're a project like liferay-amd-loader, you won't need
If you look in the next PR (#114) you'll see what it means for projects that just add |
Music to my ears ❤️ Fire at will!! |
As reported here:
#91 (comment)
Our use of the `@typescript-eslint/parser` (since 50a4d99) means
that we have a runtime dependency on `typescript` package. This worked
fine in liferay-portal, where `typescript` is already present, but not
in liferay-learn, where it is not. And I didn't know that
`@typescript-eslint/parser` doesn't actually declare all of its
transitive dependencies; as noted in:
#113
`@typescript-eslint/typescript-estree` does an unguarded `import` of
`typescript` even though it is not declared in its dependencies (they
declare it as an optional peer dependency).
So, let's not only fix this, by explicitly declaring our dependency on
TypeScript, but also declare all of our TS-related dependencies. This
will allow us to delete the devDependencies currently declared here:
https://github.com/liferay/liferay-portal/blob/f1a2b2ce06fa050d5e1ab193728ab9ec06df4315/modules/apps/remote-app/remote-app-client-js/package.json#L3-L6
docs: note that `ant setup-sdk` re-caches Gradle properties
This isn't strictly necessary, but it is an accurate description of the requirements of the project. We go with the somewhat unsatisfying range of "*" to to minimize the risk of introducing duplicates in a consuming project. At the moment, we're using v4+ in this monorepo, 3.9.7 in liferay-portal, and the lowest range that we're currently using in any other project is "^3.6.3" (in liferay-js-toolkit). In the future, we should be able to converge on v4+.
If you have a ".ts" file in your project, you are going to have
typescriptin yourdevDependencies, and ouroverridesconfiguration is going to use@typescript-eslint/parser, which in turn is going to requiretypescript, transitively, in@typescript-eslint/typescript-estreehere.And all will work, even though
@typescript-eslint/typescript-estreedoesn't explicitly declare its hard dependency ontypescriptin itspackage.json.But if you don't have any ".ts", then you shouldn't need to have
typescriptin your dependency graph, and we won't use@typescript/eslint-parser. So, it makes sense for us to call this anpeerDependencyand mark it asoptionalas per these posts:(Note that the
peerDependenciesMetafield isn't documented yet.)You could also argue that this should be under
optionalDependencies, but I think "optional peer" comes closer to the reality here.optionalDependenciesis for things that you can add, but the package should work if they're not there. That's not the case here. The package won't work if you have ".ts" files but notypescript.Related: #91 (comment)