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 eslint rules #3556

Merged
merged 2 commits into from Jan 24, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
206 changes: 147 additions & 59 deletions .eslintrc
@@ -1,76 +1,80 @@
{
"parser": "babel-eslint", // now for the support of allowImportExportEverywhere
"parser": "babel-eslint",
// "extends": [
// "plugin:jsx-a11y/recommended"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pulls in default accessibility recommendations for React components, which I think is a requirement for Reaction, right? This, like the rules, can be uncommented in a later ticket. There are ~100 errors from it, and they'll take manual work to solve.

// ],
"env": {
"browser": true,
"node": true,
"es6": true,
"mocha": true
"jest/globals": true,
"mocha": true,
"node": true
},
"globals": {
"browser": true,
"Assets": true, // Meteor global
"Package": true, // Meteor global
"FS": true, // gridFS,
"Alerts": true, // this needs to get cleaned up, but ignoring for now
"jest": true,
"expect": true,
"test": true
"Alerts": true // this needs to get cleaned up, but ignoring for now
},
"parserOptions": {
"ecmaVersion": 6,
"sourceType": "module",
"ecmaFeatures": {
"arrowFunctions": true,
"blockBindings": true,
"classes": true,
"defaultParams": true,
"destructuring": true,
"forOf": true,
"generators": false,
"modules": true,
"objectLiteralComputedProperties": true,
"objectLiteralDuplicateProperties": false,
"objectLiteralShorthandMethods": true,
"objectLiteralShorthandProperties": true,
"spread": true,
"superInFunctions": true,
"templateStrings": true,
"jsx": true
},
"allowImportExportEverywhere": true
"settings": {
"import/resolver": "meteor"
},
"plugins": ["react"],
// NOTE: We're now using eslint-4
"rules": {
"parserOptions": {
"ecmaVersion": 2017,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Diff looks weird here, but the only actual change to parserOptions is changing ecmaVersion to 2017

"sourceType": "module",
"ecmaFeatures": {
"arrowFunctions": true,
"blockBindings": true,
"classes": true,
"defaultParams": true,
"destructuring": true,
"forOf": true,
"generators": false,
"modules": true,
"objectLiteralComputedProperties": true,
"objectLiteralDuplicateProperties": false,
"objectLiteralShorthandMethods": true,
"objectLiteralShorthandProperties": true,
"spread": true,
"superInFunctions": true,
"templateStrings": true,
"jsx": true
},
"allowImportExportEverywhere": true
},
"plugins": ["import", "jest", "jsx-a11y", "react"],
"rules": {
/**
* Strict mode
* babel inserts "use strict"; for us
* http://eslint.org/docs/rules/strict
*/
* Strict mode
* babel inserts "use strict"; for us
* http://eslint.org/docs/rules/strict
*/
"strict": 0,

/**
* ES6
*/
* ES6
*/
"object-curly-spacing": ["error", "always", { "objectsInObjects": true }],
"no-var": 2, // http://eslint.org/docs/rules/no-var

/**
* Variables
*/
* Variables
*/
"no-shadow": 2, // http://eslint.org/docs/rules/no-shadow
"no-shadow-restricted-names": 2, // http://eslint.org/docs/rules/no-shadow-restricted-names
"no-const-assign": 2, // http://eslint.org/docs/rules/no-const-assign
"no-unused-vars": [2, { // http://eslint.org/docs/rules/no-unused-vars
"vars": "local",
"args": "after-used"
"vars": "all",
"args": "after-used",
"ignoreRestSiblings": true
}],
"no-use-before-define": [2, "nofunc"], // http://eslint.org/docs/rules/no-use-before-define
"no-implicit-globals": 2, // http://eslint.org/docs/rules/no-implicit-globals

/**
* Possible errors
*/
"comma-dangle": [2, "never"], // http://eslint.org/docs/rules/comma-dangle
* Possible errors
*/
"comma-dangle": ["error", "never"], // http://eslint.org/docs/rules/comma-dangle
"no-cond-assign": [2, "always"], // http://eslint.org/docs/rules/no-cond-assign
"no-console": 2, // http://eslint.org/docs/rules/no-console
"no-debugger": 1, // http://eslint.org/docs/rules/no-debugger
Expand All @@ -80,7 +84,7 @@
"no-duplicate-case": 2, // http://eslint.org/docs/rules/no-duplicate-case
"no-empty": 2, // http://eslint.org/docs/rules/no-empty
"no-ex-assign": 2, // http://eslint.org/docs/rules/no-ex-assign
"no-extra-boolean-cast": 0, // http://eslint.org/docs/rules/no-extra-boolean-cast
"no-extra-boolean-cast": 2, // http://eslint.org/docs/rules/no-extra-boolean-cast
Copy link
Contributor

Choose a reason for hiding this comment

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

"no-extra-semi": 2, // http://eslint.org/docs/rules/no-extra-semi
"no-func-assign": 2, // http://eslint.org/docs/rules/no-func-assign
"no-inner-declarations": 2, // http://eslint.org/docs/rules/no-inner-declarations
Expand All @@ -93,8 +97,8 @@
"block-scoped-var": 0, // http://eslint.org/docs/rules/block-scoped-var

/**
* JSX / React
*/
* JSX / React
*/
"jsx-quotes": [2, "prefer-double"], // http://eslint.org/docs/rules/jsx-quotes
"react/no-deprecated": 1,
"react/display-name": 1,
Expand Down Expand Up @@ -129,8 +133,8 @@
"react/sort-comp": 1,

/**
* Best practices
*/
* Best practices
*/
"consistent-return": [0, { // http://eslint.org/docs/rules/consistent-return
"treatUndefinedAsUnspecified": true
}],
Expand Down Expand Up @@ -166,7 +170,7 @@
"no-param-reassign": 2, // http://eslint.org/docs/rules/no-param-reassign
"no-proto": 2, // http://eslint.org/docs/rules/no-proto
"no-redeclare": 2, // http://eslint.org/docs/rules/no-redeclare
"no-return-assign": 2, // http://eslint.org/docs/rules/no-return-assign
"no-return-assign": ["error", "always"], // http://eslint.org/docs/rules/no-return-assign
"no-script-url": 2, // http://eslint.org/docs/rules/no-script-url
"no-self-compare": 2, // http://eslint.org/docs/rules/no-self-compare
"no-sequences": 2, // http://eslint.org/docs/rules/no-sequences
Expand All @@ -175,7 +179,7 @@
"no-undef": 2,
"radix": 2, // http://eslint.org/docs/rules/radix
"vars-on-top": 2, // http://eslint.org/docs/rules/vars-on-top
"wrap-iife": [2, "any"], // http://eslint.org/docs/rules/wrap-iife
"wrap-iife": ["error", "outside", { "functionPrototypeMethods": false }], // http://eslint.org/docs/rules/wrap-iife
"yoda": 2, // http://eslint.org/docs/rules/yoda
"max-len": [1, 160, 2, {
"ignoreComments": true,
Expand All @@ -196,18 +200,17 @@
}
}], // http://eslint.org/docs/rules/valid-jsdoc
"quote-props": [2, "consistent-as-needed"], // http://eslint.org/docs/rules/quote-props

/**
* Style
*/
* Style
*/
"indent": [2, 2, {"SwitchCase": 1}], // http://eslint.org/docs/rules/indent
"brace-style": [2, // http://eslint.org/docs/rules/brace-style
"1tbs", {
"allowSingleLine": true
}
],
"quotes": [
2, "double", "avoid-escape" // http://eslint.org/docs/rules/quotes
],
"quotes": ["error", "double", { "avoidEscape": true }], // http://eslint.org/docs/rules/quotes
"camelcase": [2, { // http://eslint.org/docs/rules/camelcase
"properties": "always"
}],
Expand Down Expand Up @@ -253,6 +256,91 @@
"space-infix-ops": 2, // http://eslint.org/docs/rules/space-infix-ops
"space-in-parens": [2, "never"], // http://eslint.org/docs/rules/space-in-parens
"spaced-comment": [2, "always"], // http://eslint.org/docs/rules/spaced-comment
"arrow-spacing": [2, { "before": true, "after": true }] // https://eslint.org/docs/rules/arrow-spacing
"arrow-spacing": [2, { "before": true, "after": true }], // https://eslint.org/docs/rules/arrow-spacing

/**
* Additional rules to enable one by one
*/
// "array-bracket-spacing": ["error", "never"],
// "array-callback-return": ["error", { "allowImplicit": true }],
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be super helpful. 👍

// "arrow-body-style": ["error", "as-needed", { "requireReturnForObjectLiteral": false }],
// "arrow-parens": [ "error", "always", { "requireForBlockBody": true }],
// "block-spacing": ["error", "always"],
// "computed-property-spacing": ["error", "never"],
// "dot-location": ["error", "property"],
// "function-paren-newline": ["error", "multiline"],
// "import/export": "error",
Copy link
Contributor

Choose a reason for hiding this comment

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

@aldeed Do these import rules come from benmosher's eslint-plugin-import plugin? https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The plugin packages all begin with eslint-plugin- in the package.json, and then the part of the name after eslint-plugin- is referenced in the "plugins" array in this file. Any rule in import/ namespace is from the import plugin package, etc.

I pulled in the rules Airbnb uses for import/, but then changed/removed a few to match Meteor/Reaction conventions. These will mostly help catch consistency things (like order of import statements) and potential issues (like mutable exports).

I added the eslint-import-resolver-meteor package, too, which makes it understand Meteor's way and be able to resolve absolute import paths.

// "import/first": ["error", "absolute-first"],
Copy link
Contributor

Choose a reason for hiding this comment

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

How will this play with dynamic imports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per the docs, it only looks at import lines, not require(), so I don't think it would look at dynamic imports either. Which is probably what we want.

// "import/newline-after-import": "error",
// "import/no-duplicates": "error",
// "import/no-mutable-exports": "error",
// "import/no-named-default": "error",
// "new-parens": "error",
// "newline-per-chained-call": ["error", { "ignoreChainWithDepth": 4 }],
Copy link
Contributor

Choose a reason for hiding this comment

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

4 is sensible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just copied this one from Airbnb, so I'm ok with any number we agree on

// "no-await-in-loop": "error",
// "no-bitwise": "error",
// "no-case-declarations": "error",
// "no-confusing-arrow": ["error", { "allowParens": true }],
// "no-empty-pattern": "error",
// "no-lonely-if": "error",
// // This rule copied from Airbnb's config
// "no-mixed-operators": ["error", {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

// "groups": [
// ["%", "**"],
// ["%", "+"],
// ["%", "-"],
// ["%", "*"],
// ["%", "/"],
// ["**", "+"],
// ["**", "-"],
// ["**", "*"],
// ["**", "/"],
// ["&", "|", "^", "~", "<<", ">>", ">>>"],
// ["==", "!=", "===", "!==", ">", ">=", "<", "<="],
// ["&&", "||"],
// ["in", "instanceof"]
// ],
// "allowSamePrecedence": false
// }],
// "no-multi-assign": ["error"],
// "no-multi-spaces": ["error", { "ignoreEOLComments": false }],
// "no-plusplus": "error",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm good with this one.

// "no-prototype-builtins": "error",
// "no-tabs": "error",
// "no-undef-init": "error",
// "no-unneeded-ternary": ["error", { "defaultAssignment": false }],
// "no-unsafe-finally": "error",
// "no-useless-computed-key": "error",
// "no-useless-concat": "error",
// "no-useless-constructor": "error",
// "no-useless-escape": "error",
// "no-void": "error",
// "object-curly-newline": ["error", { "ObjectExpression": { "multiline": true, "consistent": true }, "ObjectPattern": { "multiline": true, "consistent": true } }],
// "object-property-newline": ["error", { "allowAllPropertiesOnSameLine": true }],
// "object-shorthand": ["error", "always"],
// "operator-assignment": ["error", "always"],
// "prefer-arrow-callback": ["error", { "allowNamedFunctions": false, "allowUnboundThis": true }],
// "prefer-destructuring": ["error", {
// "VariableDeclarator": {
// "array": false,
// "object": true
// },
// "AssignmentExpression": {
// "array": true,
// "object": true
// }
// }, {
// "enforceForRenamedProperties": false
// }],
// "prefer-rest-params": "error",
// "prefer-spread": "error",
// "prefer-template": "error",
// "rest-spread-spacing": ["error", "never"],
// "space-unary-ops": ["error", {
// "words": true,
// "nonwords": false,
// "overrides": {}
// }],
// "template-curly-spacing": "error"
}
}
Expand Up @@ -4,15 +4,14 @@ import classnames from "classnames";
import { Components, registerComponent } from "@reactioncommerce/reaction-components";
import { SortableTable } from "/imports/plugins/core/ui/client/components";


const GroupsTable = (props) => {
const { group } = props;
const fields = ["name", "email", "createdAt", "dropdown", "button"];

const tableClass = (length) => {
return classnames({
"accounts-group-table": true,
"empty-table": !Boolean(length)
"empty-table": !length
});
};

Expand Down
Expand Up @@ -136,6 +136,8 @@ export function groupPermissions(packages) {

function getPermissionMap(permissions) {
const permissionMap = {};
permissions.forEach((existing) => (permissionMap[existing.permission] = existing.label));
permissions.forEach(({ label, permission }) => {
permissionMap[permission] = label;
});
return permissionMap;
}
32 changes: 0 additions & 32 deletions imports/plugins/core/accounts/client/helpers/templates.js
Expand Up @@ -26,35 +26,3 @@ Template.registerHelper("displayName", function (displayUser) {
}
}
});

/*
* registerHelper fName
*/
Template.registerHelper("fName", function (displayUser) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that (a) the exact same helper was registered in two places, and (b) I chose to remove it rather than fix the lint error in it. I do not see where this helper is used anywhere.

const user = displayUser || Meteor.user();
if (user && user.profile && user.profile.name) {
return user.profile.name.split(" ")[0];
} else if (user && user.username) {
return user.username.name.split(" ")[0];
}
if (user && user.services) {
const username = (function () {
switch (false) {
case !user.services.twitter:
return user.services.twitter.first_name;
case !user.services.google:
return user.services.google.given_name;
case !user.services.facebook:
return user.services.facebook.first_name;
case !user.services.instagram:
return user.services.instagram.first_name;
case !user.services.pinterest:
return user.services.pinterest.first_name;
default:
return i18next.t("accountsUI.guest", { defaultValue: "Guest" });
}
})();
return username;
}
return i18next.t("accountsUI.signIn", { defaultValue: "Sign in" });
});
@@ -1,4 +1,3 @@
import { Meteor } from "meteor/meteor";
import { Template } from "meteor/templating";
import { Accounts } from "meteor/accounts-base";
import { Roles } from "meteor/alanning:roles";
Expand Down Expand Up @@ -26,36 +25,3 @@ Template.registerHelper("displayName", function (displayUser) {
}
}
});

/**
* registerHelper fName
*/

Template.registerHelper("fName", function (displayUser) {
const user = displayUser || Meteor.user();
if (user && user.profile && user.profile.name) {
return user.profile.name.split(" ")[0];
} else if (user && user.username) {
return user.username.name.split(" ")[0];
}
if (user && user.services) {
const username = (function () {
switch (false) {
case !user.services.twitter:
return user.services.twitter.first_name;
case !user.services.google:
return user.services.google.given_name;
case !user.services.facebook:
return user.services.facebook.first_name;
case !user.services.instagram:
return user.services.instagram.first_name;
case !user.services.pinterest:
return user.services.pinterest.first_name;
default:
return i18next.t("accountsUI.guest", { defaultValue: "Guest" });
}
})();
return username;
}
return i18next.t("accountsUI.signIn", { defaultValue: "Sign in" });
});