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 - enforces many more rules. #139

Merged
merged 8 commits into from
Oct 7, 2015

Conversation

ColinEberhardt
Copy link
Contributor

Based on the review comments I saw for #138 I thought the linting rules could do with being 'tightened up'.

I checked the .eslintrc and found that the only thing being checked was the quotes style, and that was only being applied to Layout.js!

I've auto-configured ESLint based on the existing codebase, then added a few rules that I think are most important. Let me know what you think.

@@ -136,11 +136,11 @@ var computeLayout = (function() {
case 'column-reverse': value = node.style.marginTop; break;
}

if (value != null) {
if (value !== null && typeof value !== 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should do

value !== undefined

The typeof check is when the variable may or may not exist. This is useful for checking globals. But in this case we know it exists so we can just check against the undefined value.

@ColinEberhardt
Copy link
Contributor Author

@vjeux @devongovett - I've relaxed the eqeqeq rule to permit eqeq checks against null, as @devongovett rightly points out this allows you to check for both null and undefined with a single test.

@ColinEberhardt
Copy link
Contributor Author

As an side, it does feel like the getLeadingMargin function, and its neighbours, share a lot of common code. I'll have a look at refactoring as a separate PR.

@sophiebits
Copy link
Contributor

See how React and Relay have eslint set up – they use a common config from the fbjs-scripts package.

@ColinEberhardt
Copy link
Contributor Author

@spicyj thanks for the heads up, didn't realise there was a centralised lint configuration. I've updated the PR to use the fbjs-scripts eslint configuration and fixed all errors / warnings.

@@ -410,7 +410,7 @@ var computeLayout = (function() {
// When the user specifically sets a value for width or height
function setDimensionFromStyle(node, axis) {
// The parent already computed us a width or height. We just skip it
if (node.layout[dim[axis]] != null) {
if (typeof node.layout[dim[axis]] !== 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

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

please use x === undefined instead of typeof x === 'undefined'

@vjeux
Copy link
Contributor

vjeux commented Oct 7, 2015

Sweet! Outside of the typeof undefined and null, looks very good! Thanks for doing that

@ColinEberhardt
Copy link
Contributor Author

please use x === undefined instead of typeof x === 'undefined'

Fair point, guarding against redefinition of undefined is a little unnecessary, and does make the code less readable. I've made these updates.

Thanks for doing that

No problems :-)

@vjeux
Copy link
Contributor

vjeux commented Oct 7, 2015

Feel free to land if travis passes :)

@ColinEberhardt
Copy link
Contributor Author

Travis is happy - all done 👍

ColinEberhardt added a commit that referenced this pull request Oct 7, 2015
Eslint - enforces many more rules.
@ColinEberhardt ColinEberhardt merged commit fbeef45 into facebook:master Oct 7, 2015
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.

4 participants