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

Should no-redeclare also check functions? #2953

Closed
dominicbarnes opened this issue Jul 8, 2015 · 9 comments

Comments

Projects
None yet
4 participants
@dominicbarnes
Copy link
Contributor

commented Jul 8, 2015

I just got burned by a stupid mistake, but I think it should've been caught by no-redeclare.

Given the following code:

var a = true;
function a() {}

If I turn on no-redeclare: 2, this is not treated as an error, but I think it should.

To be clear, this was a stupid mistake on my part. I added a var to the top of a file that was named the same as a function declared much further down in my code. Nonetheless, I think no-redeclare should take hoisted functions into account as well.

@gyandeeps gyandeeps added the triage label Jul 8, 2015

@gyandeeps

This comment has been minimized.

Copy link
Member

commented Jul 8, 2015

I know there have been conversations around this. Have you tried using no-shadow rule.
Related discussions happened here: #2645

@dominicbarnes

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2015

This is not an issue no-shadow will catch, since this all happened in the "global scope" (not truly global, but it wasn't in a nested function of my module)

@gyandeeps

This comment has been minimized.

Copy link
Member

commented Jul 8, 2015

What all env are you using in your eslint config?

@dominicbarnes

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2015

In this particular case, I'm using browser (plus all ES6 features)

@nzakas

This comment has been minimized.

Copy link
Member

commented Jul 9, 2015

Yeah, most likely a bug even though the docs would indicate otherwise. We should fix this and update the docs to be more accurate.

@nzakas nzakas added bug rule accepted and removed triage labels Jul 9, 2015

@mysticatea

This comment has been minimized.

Copy link
Member

commented Jul 9, 2015

I tried:

> eslint -v
v0.24.0
> echo "var a = true; function a() {}" | eslint --stdin --env browser --env es6 --rule "no-redeclare:2" --reset --no-eslintrc

<text>
  1:24  error  a is already defined  no-redeclare

✖ 1 problem (1 error, 0 warnings)

If you have turned ecmaFeatures: {modules: true} on, yes, the no-redeclare rule had been missing global declarations. And it was fixed on master branch now. (I want to turn modules on at one-liner...)

Related: #2903

@dominicbarnes

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2015

I can verify that master has resolved this. Should I close this issue?

@gyandeeps

This comment has been minimized.

Copy link
Member

commented Jul 9, 2015

If you have verified this then you can close.

@dominicbarnes

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2015

Actually, could we add tests for no-redeclare that include checking named/hoisted functions? (just noticed all the examples there are either var or class) For example:

var a; function a() {}

dominicbarnes added a commit to segmentio/eslint-config that referenced this issue Jul 9, 2015

Adds no-redeclare to our rules
When we deployed the app, a bug I introduced in segmentio/app#e3fa8a1886cb17e8a3765d70c215b42591729c0d prevented the page from loading for all users. We needed to rollback, and I introduced a fix segmentio/app#7fb59a0602d55d8095c249d583fab6ddd75a975d that has fixed the issue.

The problem was that I accidentally created a conflicting variable in the global scope for that module. (`var` up top, `function` way later) By adding `no-redeclare`, this will catch that same problem in the future. As noted in eslint/eslint#2953, this has already been fixed in `master`, so now we just await a new release. In the meantime, adding this rule will still be a good idea as it will catch other similar problems.

ilyavolodin added a commit that referenced this issue Jul 9, 2015

Merge pull request #2962 from dominicbarnes/no-redeclare/add-tests
Update: adding some tests for no-redeclare to test named functions (fixes #2953)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.