ESCONF-44 Update eslint-react-plugin to latest v7

[alb3rtino]

https://folio-org.atlassian.net/browse/ESCONF-44

The version of eslint-plugin-react is currently fixed at 7.30.1. Since then the plugin has received numerous updates. By updating it to the latest v7 version developers can benefit from updated and added linting rules.

[github-actions]

Jest Unit Test Statistics

0 tests  ±0   0 ✔️ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ❌ ±0 

Results for commit 4c06a33. ± Comparison against base commit b34e314.

[github-actions]

BigTest Unit Test Statistics

0 tests  ±0   0 ✔️ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ❌ ±0 

Results for commit 4c06a33. ± Comparison against base commit b34e314.

[MikeTaylor]

My only question here is that of Chesterton's Fence: before you tear it down, you must first understand why it was erected. Do you know why, before now, the dependency was on a specific version rather than a range? And are you confident that the reasons in favour of changing this supersede those that made it as it is?

[MikeTaylor]

It seems that @zburke added the previous version of the lines in bb1c853 but that was an upgrade replacing an earlier fixed version. Zak, do you remember the reasoning?

[zburke]

@MikeTaylor , @alb3rtino : We use a fixed version like 7.30.1 instead of a semver range like ^7.30.1 to avoid surprising changes to lint rules, a problem that has bitten us in the past with some of the AirBnB rules. Imagine you run lint on Monday when v1.2.3 is out and your code is clean ✅ . On Tuesday, v1.3.0 comes out and changes some things. On Wednesday, your colleague pulls down your branch for review, installs deps and runs lint, and your code is not clean even though it hasn't changed ❌ . A good counter-argument is, "That's not a problem in eslint-config-stripes; your repo should commit a yarn.lock file." That's true, and it is arguably a better way to avoid bad surprises imported from updates in third-party-libraries. At the same time, until committing yarn.lock becomes standard practice across all 70+ repos, using fixed versions in eslint-config-stripes is a very easy way achieve the same thing.

So. The perfect solution of committing yarn.lock everywhere is hard to achieve. Is there a good solution that allows us to update this out-of-date stuff? I think yes, if you're cautious.

1. Stick to fixed versions instead of semver ranges for now. In repos you control, commit your yarn.lock files.
2. As a test, put these updates in the resolutions sections of a few ui-* and stripes-* repos and run lint. If they remain clean, then you have reasonable confidence that it's a "safe" change to make here. Before you commit the PR, post to Slack#stripes-updates about your plan, sth like,

We're about to update some lint-related dependencies in eslint-config-stripes [link to PR]. We tested it and think it's safe, but if you suddenly encounter lint problem that weren't there before please let us know!

and then merge. If folks squawk, be prepared/willing to either revert this change or offer to help deal with new rules in the affected repositories.

Long live Chesterson!

[zburke]

Specifically, the original reasoning of the previous update is spelled out in the ticket, ESCONF-21: the old rules were incorrectly flagging legit code.

[alb3rtino]

Thank you for the explanation and suggestions @zburke.

I do not agree that the changes made here need to be "safe". This repository sets the coding standards for most FOLIO UI projects, so it is essential that the dependencies in use here are up-to-date so that they align with the latest best practices and conventions. Failing to update these dependencies will, over time, lead to degraded code quality in all dependent projects.

Also, when projects depend on "eslint-config-stripes": "^7.0.0", they are explicitly subscribing to receive ESlint related updates, including changes to linting rules. Potential inconsistencies between builds is a problem that, as you described, needs to be addressed in the dependent projects.

I did run some tests together with PR#129 on a couple projects as you suggested with mixed results. Linting errors in some projects are to be expected and we should be prepared to assist teams in resolving any conflicts.

What about using a careted version of eslint-plugin-react and start using a yarn.lock file in this repository?

[zburke]

Also, when projects depend on "eslint-config-stripes": "^7.0.0", they are explicitly subscribing to receive ESlint related updates, including changes to linting rules.

A ^ dependency is backwards compatible. Thus, changes pulled into non-major updates of eslint-config-stripes must be "safe", i.e. must not issue new errors about code that was previously considered clean. Minor-version updates may add support for new behavior, as was the case with ESCONF-21 when we bumped eslint-plugin-react to stop it from whining about completely valid gDSFP code. If code that was previously acceptable becomes unacceptable, however, this is a breaking change.

Look, I understand and agree with you about the need to update dependencies regularly, but this PR puts the cart before the horse, changing the rules for existing code without giving consumers of this repository a way to opt out. Either we need to get everybody's yarn.lock files committed first, or this change needs to come in as a major-version change.