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

eslint for...in / for...of false positive #7566

Closed
shiraze opened this issue Aug 19, 2019 · 9 comments · Fixed by #7662
Closed

eslint for...in / for...of false positive #7566

shiraze opened this issue Aug 19, 2019 · 9 comments · Fixed by #7662

Comments

@shiraze
Copy link

shiraze commented Aug 19, 2019

Describe the bug

ESlint shows a no-unused-vars false positive with for-in loops.
Looks very similar to eslint/eslint#12117

This issue didn't exist in react-scripts@3.0.0

Did you try recovering your dependencies?

Yes.

Which terms did you search for in User Guide?

N/A

Environment

Unfortunately, running npx create-react-app --info throws the error shown further down.
I'm using react-scripts@3.1.1

Error from npx command
Environment Info:
(node:612) UnhandledPromiseRejectionWarning: Error: The system cannot find the path specified.

at Function.e.exports.sync (C:\Users\Shiraz.Esat\AppData\Roaming\npm\node_modules\create-react-app\node_modules\envinfo\dist\envinfo.js:1:7778)
at Object.copySync (C:\Users\Shiraz.Esat\AppData\Roaming\npm\node_modules\create-react-app\node_modules\envinfo\dist\envinfo.js:1:104976)
at Object.t.writeSync.e [as writeSync] (C:\Users\Shiraz.Esat\AppData\Roaming\npm\node_modules\create-react-app\node_modules\envinfo\dist\envinfo.js:1:123499)
at C:\Users\Shiraz.Esat\AppData\Roaming\npm\node_modules\create-react-app\node_modules\envinfo\dist\envinfo.js:1:124274
at Promise.all.then.e (C:\Users\Shiraz.Esat\AppData\Roaming\npm\node_modules\create-react-app\node_modules\envinfo\dist\envinfo.js:1:124289)
at process._tickCallback (internal/process/next_tick.js:68:7)

(node:612) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:612) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Steps to reproduce

Create a simple function in your CRA project with something like:

    export function simple() {
      const iterate = { prop: "value" };
      for (const item in iterate) {
        console.log(item);
      }
    }

Run npm start

Expected behavior

No ESLint warnings should be shown. item is used.

Actual behavior

Compiled with warnings.

./src/utils/file.js
Line 3: 'item' is defined but never used no-unused-vars

Search for the keywords to learn more about each warning.
To ignore, add // eslint-disable-next-line to the line before.

Reproducible demo

(Paste the link to an example project and exact instructions to reproduce the issue.)

@madmed88
Copy link

eslint/eslint#12117
downgrading from eslint 6.2.0 to 6.1.0 fixes the issue

@vincerubinetti
Copy link

vincerubinetti commented Aug 21, 2019

Does anyone know of a temporary work-around for this?

I tried downgrading and locking react-scripts to 3.1.0 and eslint to 6.1.0. It production-builds fine locally (though still throws compile errors, which it shouldn't after downgrading?), but on CI it throws compile errors AND fails to compile.

I've tried turning off no-unused-vars in my .eslintrc file, but it seems like react-scripts doesn't even respect the rules there?

I don't have much experience configuring create-react-app, so any insight or direction would be appreciated.


EDIT Nevermind, I was able to solve it by also locking eslint to 6.1.0 in peerDependencies and dependencies. I guess that forced all packages using eslint to be locked to 6.1.0.

esetnik added a commit to esetnik/create-react-app that referenced this issue Aug 26, 2019
@esetnik
Copy link

esetnik commented Aug 26, 2019

@vincerubinetti for a temporary workaround if you're using yarn you could try the resolutions feature.

@Shannanigans
Copy link

I am seeing the same but for a for-of loop. If this is verified think of renaming the issue so that others facing the same problems can more easily find it.

for (const item of items) {
        console.log(item);
}

@shiraze shiraze changed the title eslint for...in false positive eslint for...in / for...of false positive Sep 3, 2019
@jwhitlock
Copy link

resolutions didn't work for me, as reported on #7648. react-scripts build looks for an exact version:

There might be a problem with the project dependency tree.
It is likely not a bug in Create React App, but something you need to fix locally.

The react-scripts package provided by Create React App requires a dependency:

  "babel-eslint": "10.0.2"

Don't try to install it manually: your package manager does it automatically.
However, a different version of babel-eslint was detected higher up in the tree:

  /app/frontend/node_modules/babel-eslint (version: 10.0.3) 

Manually installing incompatible versions is known to cause hard-to-debug issues.
...

@madmed88
Copy link

madmed88 commented Sep 6, 2019

#7594 resolves this issue but it not merged/released yet. As a workaround I published it on npm and use it until a newer react-scripts is released with a fix. https://www.npmjs.com/package/@madmed88/react-scripts

@esetnik
Copy link

esetnik commented Sep 6, 2019

You can also use SKIP_PREFLIGHT_CHECK=true to avoid the issue mentioned by @jwhitlock. For the proper solution see #7594. Let’s get it merged!

@jwhitlock
Copy link

We had just one false positive in our codebase, so a pair of /* eslint-disable no-unused-vars */ / /* eslint-enable no-unused-vars */ comments worked:

mozilla-services/tecken@36a6425

@bdharrington7
Copy link

Just curious what the ETA is for publishing the patch?

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

Successfully merging a pull request may close this issue.

8 participants