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 ignores variables named status #1834

Closed
sidoshi opened this issue Mar 15, 2017 · 10 comments · Fixed by #1840
Closed

eslint ignores variables named status #1834

sidoshi opened this issue Mar 15, 2017 · 10 comments · Fixed by #1840

Comments

@sidoshi
Copy link
Contributor

sidoshi commented Mar 15, 2017

Can you reproduce the problem with latest npm?

yes. npm version: 4.1.1

Description

Linter ignores status variable name when checking for unused vars.
Not sure if this is a bug or expected behaviour.

Expected behavior

It should probably warn when I have not declared a variable named status and have this line of code:
console.log(status)

Actual behavior

Even though I have not declared any variable named status,
using it does not show any warning or errors.
it shows no-undef error on all other variables names like statuss as expected

Environment

Run these commands in the project folder and fill in their results:

  1. npm ls react-scripts (if you haven’t ejected): 0.9.0
  2. node -v: v7.7.1
  3. npm -v: 4.4.1

Then, specify:

  1. Operating system: Linux, Deepin
  2. Browser and version: any

Reproducible Demo

create a new app with:
create-react-app test-app
In index.js type: console.log(status)
It won't show any error that status is not defined but changing the variable name to statuss would show error as expected

@n3tr
Copy link
Contributor

n3tr commented Mar 15, 2017

As I understand it is expected behavior since status is a window property (https://developer.mozilla.org/en-US/docs/Web/API/Window/status) and we configure eslint env. browser on eslint-config-react-app/index.js#L29

@gaearon
Copy link
Contributor

gaearon commented Mar 15, 2017

It's still confusing and IMO we should forbid implicit window properties without window. qualifier.

@sidoshi
Copy link
Contributor Author

sidoshi commented Mar 15, 2017

I think eslint doesn't support that.
See eslint/eslint#3959 .
It seems like we would have to remove browser env and add a list of some variables to globals like localStorage.

@gaearon
Copy link
Contributor

gaearon commented Mar 15, 2017

I don't mind having our own list of globals.

@sidoshi
Copy link
Contributor Author

sidoshi commented Mar 15, 2017

I can work on this.

@gaearon
Copy link
Contributor

gaearon commented Mar 15, 2017

Sounds good!

@sidoshi
Copy link
Contributor Author

sidoshi commented Mar 17, 2017

see #1840

@Timer Timer added this to the 0.10.0 milestone Mar 27, 2017
@gaearon gaearon reopened this May 8, 2017
@gaearon
Copy link
Contributor

gaearon commented May 8, 2017

I think I was wrong about this. People are genuinely confused when “valid” globals don’t work—especially if they’re built into the browser.

I propose another strategy. Let’s go through the list of globals and pick a list of names that can genuinely be confused with local variables (e.g. status is a great candidate). It’s gonna be subjective but I’m okay with that.

@gaearon
Copy link
Contributor

gaearon commented May 14, 2017

Fixed by #2130 with an explicit blacklist.

@gaearon gaearon closed this as completed May 14, 2017
@gaearon
Copy link
Contributor

gaearon commented May 16, 2017

Please help beta test the new version that includes this change!
#2172

@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
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.

4 participants