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

Add no-shadow to eslintrc #352

Closed
wants to merge 1 commit into from
Closed

Add no-shadow to eslintrc #352

wants to merge 1 commit into from

Conversation

eritbh
Copy link
Member

@eritbh eritbh commented Aug 10, 2020

Apparently no-shadow isn't part of eslint:recommended. It should be, but that's not up to me.

This adds no-shadow to our config. This rule reports an error whenever a variable name from an upper scope is reused in a lower scope. This prevents uncertainty in which identically-named variable is actually being referenced in code. builtinGlobals ensures that we aren't shadowing the names of built-in objects, and hoist: all makes the following invalid:

if (someCondition) {
	const foo = 1;
	// errors because `foo` is declared in the upper scope, even though it comes after this block
}

const foo = 2;

@eritbh
Copy link
Member Author

eritbh commented Aug 10, 2020

hm, no kidding we didn't have this enabled before, travis reports almost 200 errors caused by this change. Marking as WIP until I get around to fixing those issues.

@eritbh eritbh marked this pull request as draft August 10, 2020 18:34
@eritbh
Copy link
Member Author

eritbh commented Sep 9, 2020

abandoning this, something about our eslint environment is wonky and marking lots of things as shadowed when they clearly aren't.

image

Probably not worth revisiting this until after #198, because that will allow us to clean up our mess of global variables somewhat. So this is a long way off from being practical.

@eritbh eritbh closed this Sep 9, 2020
@eritbh eritbh deleted the eslint-no-shadow branch September 9, 2020 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant