Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop eslint-plugin-sonarjs #434

Open
SukkaW opened this issue Sep 28, 2024 · 10 comments
Open

Drop eslint-plugin-sonarjs #434

SukkaW opened this issue Sep 28, 2024 · 10 comments

Comments

@SukkaW
Copy link

SukkaW commented Sep 28, 2024

https://pkg-size.dev/eslint-plugin-sonarjs

eslint-plugin-sonarjs includes @babel/core, vue-eslint-parser, eslint-plugin-react, eslint-plugin-import, typescript-eslint@v7 all in its dependencies, resulting in an installation size of 87 MiB. It is ridiculous.

I have tried to submit a PR to eslint-plugin-sonarjs, but due to how they organize their monorepo and publish packages on npm, it seems impossible to move those dependencies to peer dependencies.

eslint-config-upleveled only uses a few rules from eslint-plugin-sonarjs, but most of them have a replacement:

  • sonarjs/no-duplicated-branches and sonarjs/no-identical-conditions
  • sonarjs/prefer-single-boolean-return
    • There is no identical eslint rule here, but eslint-plugin-sonarjs doesn't work on type, so the rule can only detect boolean literal, which might be limited.
@karlhorky
Copy link
Member

karlhorky commented Sep 28, 2024

Interesting, thanks for the proposal!

I've been waiting on upgrading to eslint-plugin-sonarjs@^2, because I was looking for a changelog (eslint-plugin-sonarjs@1.0.4 is 11MB). Moving to a different plugin / rules is an interesting alternative... 🤔 I'll have to consider whether any of the v2 rules are good enough to stick with it (or if there are good enough alternatives)

@karlhorky
Copy link
Member

I have tried to submit a PR to eslint-plugin-sonarjs, but due to how they organize their monorepo and publish packages on npm, it seems impossible to move those dependencies to peer dependencies.

I would be interested in seeing the PR too - in case there's any chance that they would still approve and merge it, didn't see one in a first naive search

@SukkaW
Copy link
Author

SukkaW commented Sep 28, 2024

I have tried to submit a PR to eslint-plugin-sonarjs, but due to how they organize their monorepo and publish packages on npm, it seems impossible to move those dependencies to peer dependencies.

I would be interested in seeing the PR too - in case there's any chance that they would still approve and merge it, didn't see one in a first naive search

I had a local copy of the repo and was trying to understand how they manage the monorepo and publish them to npm. But after experimenting here and there I eventually realize that I will have to submit a huge PR to change their building infrastructure completely. That's why I didn't submit a PR yet.

@karlhorky
Copy link
Member

Ok thanks for the details!

cc @yassin-kammoun-sonarsource @vdiez in case you weren't aware of the size / dependency infrastructure issues with eslint-plugin-sonarjs

@SukkaW
Copy link
Author

SukkaW commented Sep 28, 2024

I've been waiting on upgrading to eslint-plugin-sonarjs@^2, because I was looking for a changelog (eslint-plugin-sonarjs@1.0.4 is 11MB).

BTW, it sounds a bit ironic as their eslint-plugin-sonarjs@1 repo (https://github.com/SonarSource/eslint-plugin-sonarjs) is very easy to contribute, but the SonarSource team has abandoned that repo and started maintaining eslint-plugin-sonarjs@2 in another repo (https://github.com/SonarSource/SonarJS) instead. The SonarJS repo has a very bad code structure and a poor infrastructure.

@vdiez
Copy link

vdiez commented Sep 30, 2024

Hello all,

thanks, @karlhorky, for the heads up. We are aware of the issues raised here.

Just to share some insights about why we migrated from the eslint-plugin-sonarjs repo to SonarJS:

  • the ESLint plugin was just a small subset of all the available rules in SonarJS
  • All new rules went directly to SonarJS, making the ESLint plugin feel kind of abandoned
  • The ESLint plugin was also imported by SonarJS, so unifying all rules in a single place made sense for us.

All the ESLint rules now sit under this folder. We are still in the process of improving v2 after this initial and drastic change, but hopefully, once we are in a stable situation, the ESLint plugin from Sonar will be a much better package than v1.

Now eslint-plugin-sonarjs v2 contains all rules from SonarJS (~300 rules instead of ~30 from v1). That was our initial goal.

Things that we have on our list:

  • A changelog needs to be put in place for the ESLint plugin.
  • S125 should use the parser from the ESLint config, instead of a built-in Babel dependency. This rule is the main offender for the dependency problem.
  • As mentioned here, the code structure is not what a public NPM package should look like. SonarJS is a Java project embedding a Node.js program (which runs ESLint). We are investigating ways to improve this.

Thanks

Edit: Some background about this change: https://community.sonarsource.com/t/will-sonarjs-rules-be-available-in-eslint-plugin-sonarjs/42955/6

@karlhorky
Copy link
Member

Looks like eslint-plugin-sonarjs@3.0.0 has been published (pkg-size reports 59MB, down from 79MB in eslint-plugin-sonarjs@2.0.0).

I still didn't find a clear location with a changelog / release notes yet.

@SukkaW
Copy link
Author

SukkaW commented Dec 3, 2024

(pkg-size reports 59MB, down from 79MB in eslint-plugin-sonarjs@2.0.0).

But the typescript is still a direct dependency, not a peer dependency. And @babel/eslint-parser, @babel/preset-env, @babel/preset-react, and @babel/preset-flow are also direct dependencies. Which is still bad IMHO (non-ESLint native parsers should be provided and configured by users, not shipped with an ESLint plugin).

@vdiez
Copy link

vdiez commented Dec 3, 2024

Hello @karlhorky and @SukkaW,

changelog is still on Jira. We are looking for a proper changelog solution.

As you can see in the release notes, the Typescript as peer dependency was supposed to be in the release, but we messed up during the release process. 3.0.1 will be released today with that fixed.

About babel, the problem is that we need a parser for rule S125(no-commented-code). As we support ESLint 8, the ESLint context object does not provide the parser object, only the string. Once we migrate to ESLint 9 we will be able to use the user's configured parser in Rule.Context.parser directly, without having to rely on our parser.

@ericmorand-sonarsource
Copy link

Hello all,

For information, we won't set typescript as a peer dependency: it does not fit the definition of a peer dependency.

However, we made a mistake by fixing the version of typescript to 5.7.2, which made every project install typescript 5.7.2 in addition to the version of typescript that the project depends on. typescript package being 22MB, basically every project ends up installing 22MB of additional useless data.

We'll fix that by lowering the constraint on the typescript version to the one that we actually support: ^5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants