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

Conversation

2 participants
@aldeed
Copy link
Member

commented Jan 23, 2018

Added many rules from Airbnb config, commented out for now pending later issues to fix, and changed a few existing rules. Fixed some issues caught by the rule changes.

@aaronjudd @spencern or anyone, let me know if you see anything you disagree with here. There will be about 3600 errors needing fixing from these rules. You can pull down, uncomment the rules, and run npm run lint to see.

After this is merged, I will write probably around 10 issues for uncommenting, aiming for ~360 errors per issue. It looks like a large percentage of these can be fixed with eslint --fix, but we need to do the commits in small, reviewable chunks.

Add eslint rules
Added many rules from Airbnb config, commented out for now pending later issues to fix, and changed a few existing rules. Fixed some issues caught by the rule changes.

@aldeed aldeed self-assigned this Jan 23, 2018

@aldeed aldeed requested a review from spencern Jan 23, 2018

"parser": "babel-eslint", // now for the support of allowImportExportEverywhere
"parser": "babel-eslint",
// "extends": [
// "plugin:jsx-a11y/recommended"

This comment has been minimized.

Copy link
@aldeed

aldeed Jan 23, 2018

Author Member

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.

// NOTE: We're now using eslint-4
"rules": {
"parserOptions": {
"ecmaVersion": 2017,

This comment has been minimized.

Copy link
@aldeed

aldeed Jan 23, 2018

Author Member

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

/*
* registerHelper fName
*/
Template.registerHelper("fName", function (displayUser) {

This comment has been minimized.

Copy link
@aldeed

aldeed Jan 23, 2018

Author Member

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.

additionals.profile.username = user.services[service].username;
const userServices = user.services;
for (const service in userServices) {
if ({}.hasOwnProperty.call(userServices, service)) {

This comment has been minimized.

Copy link
@aldeed

aldeed Jan 23, 2018

Author Member

Main lint fix here was wrapping in this if, but I also added the serviceObj const below so that there isn't so much dot property accessing.

@spencern
Copy link
Member

left a comment

I think it will dramatically improve code quality and consistency.

There are a few questions I've got and one or two rules that @aaronjudd might care to look at.

@@ -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

This comment has been minimized.

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

This comment has been minimized.

Copy link
@spencern

spencern Jan 23, 2018

Member

this should be super helpful. 👍

.eslintrc Outdated
// "array-bracket-spacing": ["error", "never"],
// "array-callback-return": ["error", { "allowImplicit": true }],
// "arrow-body-style": ["error", "as-needed", { "requireReturnForObjectLiteral": false }],
// "arrow-parens": [ "error", "as-needed", { "requireForBlockBody": true }],

This comment has been minimized.

Copy link
@spencern

spencern Jan 23, 2018

Member

This may need to be "always" here based on our prior rules.

This comment has been minimized.

Copy link
@aldeed

aldeed Jan 23, 2018

Author Member

I'm cool with that

.eslintrc Outdated
// "arrow-parens": [ "error", "as-needed", { "requireForBlockBody": true }],
// "block-spacing": ["error", "always"],
// "computed-property-spacing": ["error", "never"],
// "dot-location": ["error", "property"],

This comment has been minimized.

Copy link
@spencern

spencern Jan 23, 2018

Member

I prefer this to "object" as well. 👍

// "computed-property-spacing": ["error", "never"],
// "dot-location": ["error", "property"],
// "function-paren-newline": ["error", "multiline"],
// "import/export": "error",

This comment has been minimized.

Copy link
@spencern

spencern Jan 23, 2018

Member

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

This comment has been minimized.

Copy link
@aldeed

aldeed Jan 23, 2018

Author Member

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.

// "dot-location": ["error", "property"],
// "function-paren-newline": ["error", "multiline"],
// "import/export": "error",
// "import/first": ["error", "absolute-first"],

This comment has been minimized.

Copy link
@spencern

spencern Jan 23, 2018

Member

How will this play with dynamic imports?

This comment has been minimized.

Copy link
@aldeed

aldeed Jan 23, 2018

Author Member

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/no-mutable-exports": "error",
// "import/no-named-default": "error",
// "new-parens": "error",
// "newline-per-chained-call": ["error", { "ignoreChainWithDepth": 4 }],

This comment has been minimized.

Copy link
@spencern

spencern Jan 23, 2018

Member

4 is sensible

This comment has been minimized.

Copy link
@aldeed

aldeed Jan 23, 2018

Author Member

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

// "no-empty-pattern": "error",
// "no-lonely-if": "error",
// // This rule copied from Airbnb's config
// "no-mixed-operators": ["error", {

This comment has been minimized.

Copy link
@spencern
// }],
// "no-multi-assign": ["error"],
// "no-multi-spaces": ["error", { "ignoreEOLComments": false }],
// "no-plusplus": "error",

This comment has been minimized.

Copy link
@spencern

spencern Jan 23, 2018

Member

I'm good with this one.

.eslintrc Outdated
// "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", { "ignoreConstructors": false, "avoidQuotes": true }],

This comment has been minimized.

Copy link
@spencern

spencern Jan 23, 2018

Member

This will be a big cleanup job but will make a lot of code more consistent. What is the thought behind using avoidQuotes: true?

This comment has been minimized.

Copy link
@aldeed

aldeed Jan 23, 2018

Author Member

I copied this one from Airbnb without looking closely, but now that you bring it up, I think I'd actually prefer avoidQuotes: false. I typically use shorthand even if a property has to have quotation marks for special characters in it.

@spencern
Copy link
Member

left a comment

👍 Approved

@aaronjudd you may want to parse through this as well, but I don't see anything that breaks our current style guide.

@spencern spencern changed the base branch from master to release-1.7.0 Jan 24, 2018

@spencern spencern changed the base branch from release-1.7.0 to release-1.6.6 Jan 24, 2018

@spencern spencern merged commit 2f889f0 into release-1.6.6 Jan 24, 2018

4 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
WIP ready for review
Details
ci/circleci Your tests passed on CircleCI!
Details
security/snyk No new issues
Details

@spencern spencern deleted the aldeed-add-eslint-rules branch Jan 24, 2018

@spencern spencern referenced this pull request Jan 24, 2018

Merged

Release 1.6.6 #3565

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