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-use-before-define for variables and classes #36

Merged
merged 2 commits into from
Sep 22, 2020

Conversation

svanherk
Copy link
Contributor

Tackling a story we've had sitting around for a while. Having this linting rule would have saved us from a my courses hotfix after we toggled on some very old code triggered by a very specific edge case. Thoughts?

https://eslint.org/docs/rules/no-use-before-define

@dbatiste
Copy link
Collaborator

dbatiste commented Sep 21, 2020

I don't have a strong preference for this. In my view, hoisted variables were always a potential source of bugs. That said, this ought to be less of an issue with ES6 let/const. Adding this rule to the common config may result in some existing projects failing on linting. Is the intention to do a major version bump?

@svanherk
Copy link
Contributor Author

I don't have a strong preference for this. In my view, hoisted variables were always a potential source of bugs. That said, this ought to be less of an issue with ES6 let/const. Adding this rule to the common config may result in some existing projects failing on linting. Is the intention to do a major version bump?

Yeah we'd have to, core fails in a few places (mostly visual diff tests which declare a const helper at the bottom). But it'd just need to be a minor release since we're still version 0

Copy link
Member

@dlockhart dlockhart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely in favour of this change.

@svanherk svanherk merged commit b73321c into master Sep 22, 2020
@svanherk svanherk deleted the US118487_Add_no-use-before-define branch September 22, 2020 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants