Comment to disable deprecation sometimes produces lint error

[tcobbs-bentley]

This is an issue that has happened to me intermittently, and when it happens I can't make it stop happening, but then it will magically stop happening on its own and I can't make it happen. The issue is that I have a line of code in the mobile sample app that makes a call to a deprecated function, so above that line of code I have an eslint comment to disable the deprecation rule:

            // eslint-disable-next-line deprecation/deprecation
            return await iModel.views.getThumbnail(viewSpecId);

Sometimes the above works fine, and npm run build works fine, along with npm run start (I'm still using react-scripts). Other times it complains that ther is no such deprecation/deprecation rule. Initially this only happened during npm run start, but today I had it happen with npm run build. For now, I have rolled back my @itwin/eslint-plugin version to 4.0.0-dev.33.

[paulius-valiunas]

eslint should not run during npm build or npm start, it should only run when you explicitly run npm lint (or whatever the linting script is called in your package.json). Usually similar problems happen when people use create-react-app, and CRA is so stupid it doesn't resolve the eslint config correctly and uses its own config instead. You have to add DISABLE_ESLINT_PLUGIN=true to your build command, for example DISABLE_ESLINT_PLUGIN=true react-scripts build.

[tcobbs-bentley]

Two things:

1. Since the problem happens with npm run lint, disabling it during build and start doesn't really help.
2. I personally feel that disabling it during build and start just encourages people to accidentally check in broken code that then has to be fixed later. (Our repositories don't automatically do npm run lint during every commit.) If the problem can only be fixed to not occur during npm run lint, then we will disable the lint plugin during our build and start.

[paulius-valiunas]

1. Since the problem happens with npm run lint, disabling it during build and start doesn't really help.

Oh? Well that's different then, I thought this only happened in the build and start scripts. I guess I misunderstood your description:

Initially this only happened during npm run start, but today I had it happen with npm run build.

If it also happens when running eslint directly, please try running it with the --debug argument and when it fails, paste the output here. It might contain some hints for why the config is not being loaded correctly.

(Our repositories don't automatically do npm run lint during every commit.)

They should. ESLint is really not that slow, at least with our config. It should be part of your PR checks.

[tcobbs-bentley]

When I created this issue, npm run lint was working, but I'm pretty sure I later saw that fail as well. I'll have to test to verify that, though.

[anmolshres98]

@tcobbs-bentley any updates on this issue?

[tcobbs-bentley]

We switched to an earlier version (4.0.0-dev.33) to work around the problem. As far as I know, the problem still exists in the latest version.

[anmolshres98]

Could you comment the steps to reproduce please

[tcobbs-bentley]

The problem was intermittent for npm run lint, and today I am unable to reproduce it there. However, I definitely did have it happen to me in the past, and the problem still happens with both npm run build and npm run start. I realize that you said not to run eslint during those steps, but that is the only way I can reproduce the problem reliably. If you want to see that, do the following:

git clone https://github.com/iTwin/mobile-samples.git
cd mobile-samples
git checkout travis/itwin-eslint-dev-49
cd cross-platform/react-app
npm install
npm run build

(If you are in Windows, change the / path separators to \.)

[anmolshres98]

Thanks Travis. I was able to clone the repo update eslint-plugin to latest nightly build and I saw the error. The error was pretty consistent every time I ran npm start and npm run build. It occurred every time I ran those two scripts but never experienced the error when running npm run lint. I am on Mac for reference and ran the scripts multiple times but the error never occurred on lint command but always when building or starting. This leads me to believe that @paulius-valiunas 's hunch was correct. So I have created a PR with the switch to the new flat config and also the change paulius suggested. After implementing Paulius' suggestion, the error did not occur so I believe that solved the issue. Here's the PR: https://github.com/iTwin/mobile-samples/pull/301

[tcobbs-bentley]

Given that it only happened intermittently with npm run lint in dev.48, and given that it happens consistently with npm run build and npm run start both in dev.48 and dev.49, I strongly suspect that whatever the underlying problem is, it still exists, and will still cause npm run lint to intermittently fail.

[anmolshres98]

Based on Paulius' comment, I would believe that the error's root problem during npm run start and npm run build because react-scripts is using an older version of eslint that expects older version of eslint config but the recent eslint-plugin mandates the newer flat config and does not support traditional eslint config. This mismatch leads to react-scripts using eslint with its default config with traditional configuration (which doesn't exist after newer change on our repo) which leads to error from react-scripts since its checking for rules we didn't specify.

Is there a reason that there are no build or lint checks for PRs for that repo?

[tcobbs-bentley]

With only two people making regular contributions to that repo, it was never felt that it was worth it to learn how to create a GitHub-compatible workflow and then configure it.

Regarding react-scripts, I don't think that's the problem. According to npm ls, there is only one version of eslint in my project: 8.47.0. Having said that, unless I can reproduce the problem with npm run lint, I can't prove that the bug isn't in @bentley/react-scripts (which is of course itself deprecated).

[paulius-valiunas]

Given that it only happened intermittently with npm run lint in dev.48, and given that it happens consistently with npm run build and npm run start both in dev.48 and dev.49, I strongly suspect that whatever the underlying problem is, it still exists, and will still cause npm run lint to intermittently fail.

when you're running npm run build or npm run start, you're using a completely different config. I'm not denying the possibility that a problem exists that causes npm run lint to fail sometimes, but if it does exist, I can 100% guarantee you that it's a different problem than the one causing failures during build and start.

[anmolshres98]

With only two people making regular contributions to that repo, it was never felt that it was worth it to learn how to create a GitHub-compatible workflow and then configure it.

I understand, I have some experience with setting up github workflows in other repos before so let me know if you would like me to set that up for you. I would be happy to :)

[anmolshres98]

Closing based on this comment: https://github.com/iTwin/mobile-samples/pull/301#issuecomment-1912697990 & the reported issue is behaving correctly and as expected in the current version of this repo