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

order of variable declaration is not taken into account. #636

Closed
kilianc opened this issue Sep 24, 2016 · 14 comments
Closed

order of variable declaration is not taken into account. #636

kilianc opened this issue Sep 24, 2016 · 14 comments

Comments

@kilianc
Copy link

kilianc commented Sep 24, 2016

Not sure what rule is missing but this passes validation:

const b = a
const a = 5
@feross
Copy link
Member

feross commented Sep 26, 2016

We currently do not enable the no-use-before-define rule because it triggers way too many false positives. Once this eslint issue is addressed, we can enable the rule so that issues like the one you encountered (as well as this one) will be caught.

@LinusU
Copy link
Member

LinusU commented Sep 27, 2016

This wasn't meant to be closed, right?

@LinusU LinusU reopened this Sep 27, 2016
@feross
Copy link
Member

feross commented Sep 27, 2016

Yikes! Yeah, this was not intentional. I used the phrase "until this bug is fixed: #636" in another repo and since I have permissions on this repo, it closed the issue. Heh.

@dcousens
Copy link
Member

dcousens commented Sep 27, 2016

I personally get bitten by this issue so often I've enabled it in my global eslint-config-standard/eslintrc.json 😕

@feross
Copy link
Member

feross commented Sep 27, 2016

@dcousens My plan is once/if that eslint bug gets fixed, enable the rule so that:

  • using classes before they're defined is not allowed
  • using function declarations before they're defined is allowed (hoisting)
  • using variable declarations before they're defined is allowed, but prevented when it's clearly incorrect, e.g.

Not allowed:

console.log(foo)
var foo = 5

Allowed:

function printFoo() {
    console.log(foo)
}
var foo = 5

This is allowed since it is possible to use printFoo() correctly if you call it after foo is defined. And lots of users of standard do this.

When we start enforcing let over var, maybe we can tighten up this rule.

@dcousens
Copy link
Member

dcousens commented Sep 27, 2016

I completely agree, it makes recursive or circular functions completely impossible otherwise.
The example I often run into that annoys me locally:

function foo (x) {
  return x ? bar(x) : 1
}

function bar (x) {
  return foo(x - 1)
}

No valid ordering exists with the rule as it stands AFAIK.
It's just a frustrating situation.

edit: actually, [2, "nofunc"] does resolve this issue locally

@dcousens
Copy link
Member

dcousens commented Jan 6, 2017

Enabling

"no-use-before-define": [2, "nofunc"],
"no-unused-vars": [2, { "vars": "all", "args": "all", "argsIgnorePattern": "^_[0-9]?$" }]

Locally in a project recently, uncovered 5+ critical errors, and 30+ warnings overall that had gone unmissed in a refactoring... bleh
(the [0-9] is to allow for multiple unused arguments)

I know its blocked, but, it is overall frustrating when the potential errors here are quite frustrating and prolific...

Running the same thing against an OSS project I run, and it already found 1 potentially critical error (thankfully not released!) - bitcoinjs/bitcoinjs-lib@fd9c70d

@feross
Copy link
Member

feross commented Jan 19, 2017

@dcousens Are you proposing we just enable the overly aggressive rule even before the eslint issue is addressed? We could do that, but I'm worried about the effect of all the warnings on valid code. We don't want users to think that pleasing standard is a lot of work. It's a tough balance to maintain.

I just pinged the team to ask about the status of the blocked issue eslint/eslint#7111 - hopefully this gets it some movement.

@feross
Copy link
Member

feross commented Jan 19, 2017

The issue at eslint/eslint#7111 was just accepted by the team!

@dcousens
Copy link
Member

dcousens commented Jan 20, 2017

@feross it was more of a notice for others whom might be waiting on eslint/eslint#7111, but wanted to increase safety in their own projects until it is fixed.

@feross
Copy link
Member

feross commented Jan 27, 2017

This was just merged. Let's include it in the standard v9 release :)

@feross
Copy link
Member

feross commented Feb 9, 2017

This will be part of standard v9. No ecosystem impact.

feross added a commit to standard/eslint-config-standard that referenced this issue Feb 9, 2017
Using hoisting is still allowed, but it is prevented when clearly
incorrect, e.g.:

console.log(foo)
var foo = 1

var variable1 = variable1

Fixes: standard/standard#615
Fixes: standard/standard#636
@feross feross closed this as completed Feb 9, 2017
feross added a commit to standard/eslint-config-standard that referenced this issue Feb 9, 2017
Using hoisting is still allowed, but it is prevented when clearly
incorrect, e.g.:

console.log(foo)
var foo = 1

var variable1 = variable1

Fixes: standard/standard#615
Fixes: standard/standard#636
@caesarsol
Copy link

Sorry, is this distinguishing between var and let/const? Doe the behaviour change in any way?

@feross
Copy link
Member

feross commented Feb 9, 2017

This rule will prevent clearly wrong code, like:

console.log(x)
const x = 5

But it will allow code like:

function foo () {
  console.log(x)
}
const x = 5
foo()

Since it works fine :)

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

No branches or pull requests

5 participants