update dependencies

[juanpcapurro]

• updated most dependencies to their latest version
• didn't update to the last chalk version since it's in pure ESM
• modified calls to cosmiconfig since it's API changed exposing different objects for sync and async calls
• updated ignore to v5, and per the upgrade guide, I enabled relative paths manually.
• fixed dependency versions to avoid unexpected changes in behaviour due
  to dependencies not respecting semver versioning

[frangio]

This is great, thanks for taking the time with this.

I think there are a couple of issues:

fixed dependency versions to avoid unexpected changes in behaviour due to dependencies not respecting semver versioning

Was this motivated by a specific problem? It's generally much better to avoid pinning dependencies, and instead depend on a semver range. If a dependency or transitive dependency suddenly has a new security advisory and the fix is a new version, having the dependency declared as a semver range will let users get the new version without needing a new Solhint release. This will reduce the maintenance burden for you! Additionally, it allows installations to better deduplicate dependencies.

eslint@8

ESLint is used in require(`eslint/lib/formatters/${formatterName}`). It doesn't seem that these files are in the same place in version 8. Additionally, there is now an "exports" field in ESLint's package.json that does not export the formatters, so it's not possible to use them and some alternative is needed. Potentially the formatter files can be copy-pasted into this repo, since the license seems to allow that, and this has the added benefit that ESLint can be a dev dependency so that users of Solhint don't need to install it along with it.

[fvictorio]

ESLint is used in require(`eslint/lib/formatters/${formatterName}`). It doesn't seem that these files are in the same place in version 8.

This would've been caught by the e2e tests in the CI, but the PR was merged without running them: https://github.com/protofire/solhint/actions/runs/3951393216

If someone that is not part of the repository sends a pull request, the CI won't be run automatically. You need to click "Approve and run". It's not a good idea to merge a PR without checking that the CI passes.

[dbale-altoros]

ESLint is used in require(`eslint/lib/formatters/${formatterName}`). It doesn't seem that these files are in the same place in version 8.

This would've been caught by the e2e tests in the CI, but the PR was merged without running them: https://github.com/protofire/solhint/actions/runs/3951393216

If someone that is not part of the repository sends a pull request, the CI won't be run automatically. You need to click "Approve and run". It's not a good idea to merge a PR without checking that the CI passes.

You're absolutely right
Juan Pablo Capurro was part of protofire. I did not check if he was part of the repo as well. As soon as I pressed merge I realized about the CI

[juanpcapurro]

sorry that I broke the e2e tests 🙃 I only just realized that they were there.

I'll submit a PR fixing them and address the feedback tomorrow

[juanpcapurro]

Was this motivated by a specific problem?

Not in this particular repository, but I've had it happen a few times. I can't
think of a public example off the top of my head, and it certainly didn't
happen with big name dependencies (eslint, lodash, etc), but with smaller ones.

It's generally much better to avoid pinning dependencies, and instead depend
on a semver range. If a dependency or transitive dependency suddenly has a
new security advisory and the fix is a new version, having the dependency
declared as a semver range will let users get the new version without needing
a new Solhint release. This will reduce the maintenance burden for you!
Additionally, it allows installations to better deduplicate dependencies.

Okay, I'm semver-range-pilled, specially for the part of deduplicating dependencies.

Potentially the formatter files can be copy-pasted into this repo, since the
license seems to allow that

AFAIK license-wise it's the same to copy-paste code into the repo than to use
it as a dependency so there shouldn't be any difference

and this has the added benefit that ESLint can be a dev dependency so that
users of Solhint don't need to install it along with it.

that would be cool ✨ I peeked into the formatter's implementations and they
don't seem like something that could have a lot of updates for us tu backport.

If there's consensus, I could implement a PR which:

• make eslint's latest version a dev dependency (and ensure it's run by CI)
• remove eslint as a regular dependency, and copy the formatters we actually
  use into this codebase
• fixes the e2e tests in the process

I planned to implement

• the PR described above
• a PR setting dependencies to semver ranges after feedback by @frangio

however, I see that @dbale-altoros has uploaded #384 which both implements the
former and rolls back eslint to version 5, and #383 which reverts this PR in
it's entirety. Do you plan to take care of both issues? I don't mind
implementing both, but it would be wasteful for me to work on something that's
already being fixed by someone else.

[fvictorio]

If there's consensus, I could implement a PR which:

• make eslint's latest version a dev dependency (and ensure it's run by CI)
• remove eslint as a regular dependency, and copy the formatters we actually
  use into this codebase
• fixes the e2e tests in the process

I think this would be ideal! @dbale-altoros do you mind if we close #384 in favor of that?

[dbale-altoros]

yes , sure let's do that @juanpcapurro @fvictorio

beware that eslint changed fomaters path since version 6 so the solhint.js file requesting the formatters gives an error as frangio said (path is changed to the cli-engine folder in ver version 6 and 7.... in the latest i did nor search...)

keep in mind that when working with eslint 5x, I had to downgrade some eslint dependences but since the commander package was updated to 10 e2e test did not pass. I downgraded commander package to the same as before and e2e are now ok in the #384

[dbale-altoros]

@juanpcapurro @fvictorio I will close #383 and #384
in favor of what you proposed

[frangio]

If there's consensus, I could implement a PR which:

• make eslint's latest version a dev dependency (and ensure it's run by CI)

• remove eslint as a regular dependency, and copy the formatters we actually
  use into this codebase

Yes, this sounds great!