README step to add Angular schematic results in incorrect dependencies in package.json

[jattasNI]

I started integrating the styleguide into the SystemLink project. When I ran ng add @angular-eslint/schematics as instructed by our README, it modified the package.json and package-lock.json to include these additional dependencies:

With NPM 7, I think this behavior is incorrect:

1. The dependencies are unnecessary, since they are peer dependencies and NPM 7 could automatically install them without cluttering package.json
2. The dependency versions cause unnecssary conflicts because they are overly specific. For example, the schematic installed "@typescript-eslint/eslint-plugin": "4.28.2" but @ni/eslint-config-typescript needs version "@typescript-eslint/eslint-plugin": "^4.31.0". This results in npm install creating nested node_modules directories to handle the version conflicts and potentially some tools using an unexpected version of those dependencies.

Some possible remedies:

1. Modify the README to remove the step about ng add @angular-eslint/schematics. I don't think this is a good idea because (I think) the later step to ng g @angular-eslint/schematics:convert-tslint-to-eslint depends on this step.
2. Add a note to the README saying to revert package[-lock].json changes after adding the schematic if you're using NPM 7+
3. File a bug to @angular-eslint about the behavior. I couldn't find any existing issues

I'm interested in everyone's thoughts on this, but particularly @TrevorKarjanis.

[TrevorKarjanis]

Always use the schematic. It will do more than add dependencies, and it ensures the steps are maintained upstream. Also, these aren't peer dependencies. Even if they were, not everyone uses npm 7. As for the version match criteria, that's the responsibility of the consumer, but we can certainly open an issue/contribute upstream to improve it by matching major versions by default.

Yes, there are quite a few packages. This is why Angular declined to include it by default. It looks like it follows @typescript-eslint's pattern, and it has to account for templates. We could file an issue to propose an additional encapsulating package, but there are likely good reasons for this, like removing template support when it's not needed.

Interested in starting discussions upstream? I certainly can if we're interested in exploring that direction.

[TrevorKarjanis]

By not peer dependencies, I mean literally. If you're asking why they aren't, my guess is that it's setup to support various scenarios where this combination is not used. Again, we can always ask for context.

[jattasNI]

I tried removing all of the deps that the schematic had added. I was able to remove all except @typescript-eslint/eslint-plugin. That one needed to be explicit or else running lint would report an error because it couldn't find that ruleset to extend. I think this is because of the version conflicts and nested node_modules mentioned above.

At this point the best guidance I could suggest for the README would be something vague like "Not all dependencies added by the schematic are strictly required. If the dependencies cause conflicts or you wish to keep your package.json minimal, you may have to manually remove some or all of them."

[TrevorKarjanis]

I can and intended to take a look at improving the README. However, removing the dependencies doesn't make sense to me at the moment. I looked at all the package.json files and really only eslint and @typescript-eslint/parser are listed. The linter configuration in angular.json requires the builder, and based on its configuration we should need the @angular-eslint parts.

As for guidance, I would suggest using the major version (caret) operator which should allow an upgrade to what @ni/eslint-config-typescript requires.

[TrevorKarjanis]

Now that we're all on npm 8, I've changed my opinion and agree that these dependencies aren't necessary. It is now common at NI to not declare them. I'm addressing this in #119, but I think the best long term solution would be to provide a schematic that removes them automatically. We could also include support for configuring multi-project workspace automatically. I'm not sure why Angular ESLint doesn't support that.

[TrevorKarjanis]

Also, as part of the Angular 15 upgrade research we'll likely need to evaluate if these dependencies need to be specified when the style guide supports 14 and 15. npm commonly fails to resolve the multi-version conditions, so a user may need to explicitly declare the correct version.