Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Add eslint rules to match Firefox #2365

Closed
ianb opened this issue Mar 13, 2017 · 6 comments
Closed

Add eslint rules to match Firefox #2365

ianb opened this issue Mar 13, 2017 · 6 comments
Assignees

Comments

@ianb
Copy link
Contributor

ianb commented Mar 13, 2017

I noticed some eslint failures on import. Because of that, and just for general conformity, we should use the same rules as the Firefox tree.

@Standard8: for you?

@pdehaan
Copy link
Collaborator

pdehaan commented Mar 13, 2017

As close as I can figure, here's the mozilla-central .eslintrc.js config file:

"use strict";

module.exports = {
  // When adding items to this file please check for effects on sub-directories.
  "plugins": [
    "mozilla"
  ],
  "rules": {
    "mozilla/avoid-removeChild": "error",
    "mozilla/import-globals": "warn",
    "mozilla/no-import-into-var-and-global": "error",
    "mozilla/no-useless-parameters": "error",
    "mozilla/no-useless-removeEventListener": "error",
    "mozilla/use-default-preference-values": "error",
    "mozilla/use-ownerGlobal": "error",

    // No (!foo in bar) or (!object instanceof Class)
    "no-unsafe-negation": "error",
  },
  "env": {
    "es6": true
  },
  "parserOptions": {
    "ecmaVersion": 8,
  },
};

And digging further, here's the main source for the eslint-plugin-mozilla plugin:

"use strict";

/**
 * Based on npm coding standards at https://docs.npmjs.com/misc/coding-style.
 *
 * The places we differ from the npm coding style:
 *   - Commas should be at the end of a line.
 *   - Always use semicolons.
 *   - Functions should not have whitespace before params.
 */

module.exports = {
  "env": {
    "node": true
  },

  "rules": {
    "brace-style": ["error", "1tbs"],
    "camelcase": "error",
    "comma-dangle": ["error", "never"],
    "comma-spacing": "error",
    "comma-style": ["error", "last"],
    "curly": ["error", "multi-line"],
    "handle-callback-err": ["error", "er"],
    "indent": ["error", 2, {"SwitchCase": 1}],
    "max-len": ["error", 80, 2],
    "no-multiple-empty-lines": ["error", {"max": 1}],
    "no-undef": "error",
    "no-undef-init": "error",
    "no-unexpected-multiline": "error",
    "object-curly-spacing": "off",
    "one-var": ["error", "never"],
    "operator-linebreak": ["error", "after"],
    "semi": ["error", "always"],
    "space-before-blocks": "error",
    "space-before-function-paren": ["error", "never"],
    "keyword-spacing": "error",
    "strict": ["error", "global"],
  },

  // Globals accessible within node modules.
  "globals": {
    "DTRACE_HTTP_CLIENT_REQUEST": true,
    "DTRACE_HTTP_CLIENT_RESPONSE": true,
    "DTRACE_HTTP_SERVER_REQUEST": true,
    "DTRACE_HTTP_SERVER_RESPONSE": true,
    "DTRACE_NET_SERVER_CONNECTION": true,
    "DTRACE_NET_STREAM_END": true,
    "Intl": true,
  },
};

It's mildly unfortunate that the eslint-plugin-mozilla doesn't export a "recommended" config so we could easily just do something like this, but hey, what can you do...

extends:
  - eslint:recommended
  - plugin:mozilla/recommended

UPDATE: Filed plugin:mozilla/recommended request in bugzilla as 1347712.

@Standard8
Copy link
Contributor

@ianb - we have two options:

  1. Add a line to .eslintignorerc to ignore pageshot
  2. Update pageshot's configuration to match .eslintrc.js and toolkit/.eslintrc.js in m-c (or create a recommended & import it etc).

If we do 1, then I'd say, at a minimum, we want to also add a few more rules to pageshot's configuration, especially no-undef (there's various helpers for this).

@ianb
Copy link
Contributor Author

ianb commented Mar 14, 2017

I'd like to just integrate the entire of the eslint config. My only concern is it tends to require a big diff that conflicts with other changes, so whoever does it should try to get it done quickly once they start.

@pdehaan
Copy link
Collaborator

pdehaan commented Mar 14, 2017

In other interesting news, I can't get the latest eslint-plugin-mozilla versions to work at all.

I created https://github.com/pdehaan/eslint-mozilla yesterday and tried using latest versions and Node 6/7 and it just fails with some error about not being able to find eslint-plugin-mozilla (even though eslint and eslint-plugin-mozilla are both installed locally -- see below).

Interestingly, eslint-plugin-mozilla works in the mozilla/activity-stream repo, but it's an older version (eslint-plugin-mozilla@0.0.3). As soon as I update to eslint-plugin-mozilla@latest it starts failing with the following odd/vague error:

➜  activity-streams git:(master) ✗ $(npm bin)/eslint . --ext=.js,.jsx

Oops! Something went wrong! :(

ESLint couldn't find the plugin "eslint-plugin-mozilla". This can happen for a couple different reasons:

  1. If ESLint is installed globally, then make sure eslint-plugin-mozilla is also installed globally. A globally-installed ESLint cannot find a locally-installed plugin.

  2. If ESLint is installed locally, then it's likely that the plugin isn't installed correctly. Try reinstalling by running the following:

    npm i eslint-plugin-mozilla@latest --save-dev

If you still can't figure out the problem, please stop by https://gitter.im/eslint/eslint to chat with the team.

It looks like npm has the following published versions:

$ npm info eslint-plugin-mozilla versions
[ '0.0.3', '0.2.1', '0.2.3', '0.2.5', '0.2.28', '0.2.29' ]

Both eslint-plugin-mozilla@0.2.29 (latest) and eslint-plugin-mozilla@0.2.28 failed for me, but I did manage to get eslint-plugin@0.2.5 working locally in mozilla/activity-stream.

/cc @Standard8

@Standard8 Standard8 self-assigned this Mar 15, 2017
@Standard8
Copy link
Contributor

Ok, I'll bite, we've about 3 projects wanting to use something similar now, so it is time to come up with a better solution.

@pdehaan I thought 0.2.29 should have been working a bit better, but its possible that I missed something. I've got some ideas for how to make this work properly, so I'll start on those.

@ianb ianb added this to the Page Shot in 54 milestone Mar 15, 2017
@pdehaan
Copy link
Collaborator

pdehaan commented Mar 15, 2017

@Standard8: One small question. I seem to be stuck in eslint-plugin-mozilla/lib/environments/places-overlay.js:

var modules = require(path.join(root,
                                "tools", "lint", "eslint", "modules.json"));

This seems to be where my build is failing. I was hacking in my node_modules directly and did some garbage like this:

var root = helpers.getRootDir(module.filename);

var tmp = path.join(root, "tools", "lint", "eslint", "modules.json");

console.log(tmp);
// var modules = require(tmp);

var modules = [];

We're trying to do a dynamic require(), but logging out that tmp variable gives me the following:
/Users/pdehaan/dev/github/mozilla-services/pageshot/tools/lint/eslint/modules.json

I'm guessing that's why this is crashing hard, because I don't have a "tools/lint/eslint/modules.json" file in my Page Shot repo tree (but it looks like it's a path in the m-c repo: http://searchfox.org/mozilla-central/source/tools/lint/eslint/modules.json).


UPDATE: Submitted 1347709 in Bugzilla, like a big boy!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants