DevTools/CodingStandards

The DevTools JS code base is quite large, and a lot of different people contribute to it all the time, so it's important that a set of standards be shared when coding so that the code is consistent, written in a predictable style that also hopefully helps avoid common mistakes.

JS linting with ESLint

In order to help people write standard-compliant code from the start and avoid wasting time during code reviews, a set of ESLint configuration files have been added to the DevTools code base so that JavaScript can be analyzed automatically.

This automatic linting can happen either while coding, in a code editor, or when using the command line.

• Learn more about ESLint,
• learn how to integrate ESLint in your code editor,
• or run it on the command line. Note that ESLint has been integrated to our build system and can therefore be run on the command line with ./mach eslint.

Our ESLint rule set is meant to be used with ESLint 1.10.3 at the moment, and is not compatible with ESLint 2.0.0.

Installing ESLint and Recommended plugins

From the root of the project type:

./mach eslint --setup

NOTE: If you are not an administrator you either need to use an administrator account or use sudo depending on your operating system.

ESLint, eslint-plugin-mozilla and eslint-plugin-react will be automatically downloaded and installed.

Running ESLint from the command line

The preferred way of running ESLint from the command line is by using mach like this:

./mach eslint path/to/directory

This makes sure that ESLint runs with the same configuration that on our CI environment (see the next section).

Note that the file .eslintignore at the root of the repository contains a list of paths to ignore. This is because a lot of the code isn't ESLint compliant yet. We're in the process of making code free of ESLint warnings and errors, but this takes time. In the meantime, make sure the file or folder you are running ESLint on isn't ignored.

Running ESLint in CI

Relying only on people to run ESLint isn't enough to guarantee new warnings or errors aren't introduced in the code. Therefore, ESLint also runs automatically in our Continuous Integration environment. This means that every time a commit is pushed to one of the repositories, a job runs ESLint on the whole code.

If you are pushing a patch to the try repository to run the tests, then you can also tell it to run the ESLint job and therefore verify that you did not introduce new errors. If you build on all platforms, then the ESLint job will run by default, but if you selected a few platforms only in your trysyntax, then you need to also add eslint-gecko as a target platform for ESLint to run.

Running ESLint in SublimeText

SublimeText is a popular code editor and it supports ESLint via a couple of plugins. Here are some pointers to get started:

• make sure you have SublimeText 3, the linter plugin doesn't work with ST2,
• install SublimeLinter 3, this is a framework for linters that supports, among others, ESLint, (installing SublimeLinter via Package Control is the easiest way),
• with SublimeLinter installed, you can now install the specific ESLint plugin (the installation instruction provide details about how to install node, npm, eslint which are required).
• make sure to configure SublimeLinter with the --no-ignore option so that errors are also shown for source files that are ignored. To do this, open the SublimeLinter user configuration at: Preferences / Package Settings / SublimeLinter / Settings - User, and add "args": "--no-ignore" to the eslint linter object.

Once done, open the mozilla project in SublimeText and open any JS file in either /browser/devtools or /toolkit/devtools (the 2 main directories for DevTools code). You can then trigger the linter via the contextual menu (right click on the file itself) or with a keyboard shortcut (ctrl+option+L on Mac).

You should see errors and warnings in the gutter as shown in the screenshot below. You can also see all errors listed with ctrl+option+A, and individual errors by clicking in the gutter marker.

Running ESLint in Emacs

• First, install the flycheck package (flymake doesn't support ESLint yet). You can get flycheck from the marmalade or melpa-stable repositories.

• Tell flycheck to disable jslint, and enable flycheck in your javascript mode. This snippet assumes the built-in javascript mode, but with minor changes (just the name of the hook) should work fine with js2-mode as well:

(defun my-js-mode-hacks ()
  ;; Set this locally so that the head.js rule continues to work
  ;; properly.  In particular for a mochitest we want to preserve the
  ;; "browser_" prefix.
  (when (buffer-file-name)
    (let ((base (file-name-nondirectory (buffer-file-name))))
      (when (string-match "^\\([a-z]+_\\)" base)
	(setq-local flycheck-temp-prefix (match-string 1 base)))))
  (flycheck-mode 1))
(require 'flycheck)
(setq-default flycheck-disabled-checkers
	      (append flycheck-disabled-checkers
		      '(javascript-jshint)))
(add-hook 'js-mode-hook #'my-js-mode-hacks)

• flycheck puts its bindings on C-c ! by default, so use C-c ! C-h to see what is available. There are key bindings to list all the errors and to move through the errors, among other things.

Getting Rid of ESLint Errors

Here is list of advice for people writing new code that, hopefully, help with writing eslint-clean code:

• When moving or refactoring a piece of code, consider this as an opportunity to remove all ESlint errors from this piece of code. In fact, it may even be a good opportunity to remove all ESLint errors from the entire file.
• When doing ESLint-only changes, please do them in a separate patch from the actual functionality changes or bug fix. This helps make the review easier, and isolate the actual changes when looking at the source history.
• When cleaning an entire file or folder from ESLint errors, do not forget to remove the corresponding entry from the .eslintignore file.
• When writing new code, from scratch, please make it ESLint compliant from the start. This is a lot easier than having to revisit it later.
• ESLint also runs on <script> tags in HTML files, so if you create new HTML test files for mochitests for example, make sure that JavaScript code in those files is free of ESLint errors.
• Depending on how a dependency is loaded into a file, the symbols this dependency exports might not be considered as defined by ESLint. For instance, using Cu.import("some.jsm") doesn't explicitly say which symbols are now available in the scope of the file, and so using those symbols will be consider by ESLint as using undefined variables. When this happens, please avoid using the /* globals ... */ ESLint comment (which tells it that these variables are defined). Instead, please use /* import-globals-from relative/path/to/file.js */. This way, you won't have a list of variables to maintain manually, the globals are going to be imported dynamically instead.
• In test files (xpcshell and mochitest), all globals from the corresponding head.js file are imported automatically, so you don't need to define them using a /* globals ... */ comment or a /* import-globals-from head.js */ comment.

Code style

Probably the best piece advice there is when talking about the code style is be consistent with the rest of the code in the file.

The primary source for the style guide is the .eslintrc; but for quick reference here are some of the main code style rules:

• file references to browser globals such as window and document need /* eslint-env browser */ at the top of the file
• lines should be 80 characters maximum (go longer if wrapping hurts readability),
• indent with 2 spaces (no tabs!),
• camelCasePlease,
• don't open braces on the next line,
• don't name function expressions: let o = { doSomething: function doSomething() {} };,
• aim for short functions, 24 lines max (ESLint has a rule that checks for function complexity too),
• aArguments aAre the aDevil (don't use them please),
• "use strict"; globally per module,
• semicolons; // use them,
• no comma-first,
• consider using Task.jsm (Task.async, Task.spawn) for nice-looking asynchronous code instead of formatting endless .then chains,
• use ES6 syntax:
  • function setBreakpoint({url, line, column}) { ... },
  • (...args) => { } rest args are awesome, no need for arguments,
  • for..of loops,
• don't use non-standard SpiderMonkey-only syntax:
  • no for each loops,
  • no function () implicitReturnVal,
  • getters / setters require { },
• only import specific, explicitly-declared symbols into your namespace:
  • const { foo, bar } = require("foo/bar");,
  • const { foo, bar } = Cu.import("…", {});,
• use Maps, Sets, WeakMaps when possible,
• use `template strings` whenever possible to avoid concatenation, allow multi-line strings, and interpolation

Comments

Commenting code is important, but bad comments can hurt too, so it's important to have a few rules in mind when commenting:

• If the code can be rewritten to be made more readable, then that should be preferred over writing an explanation as a comment.
• Instead of writing a comment to explain the meaning of a poorly chosen variable name, then rename that variable.
• Avoid long separator comments like // ****************** another section below **********. They are often a sign that you should split a file in multiple files.
• Line comments go above the code they are commenting, not at the end of the line.
• Sentences in comments start with a capital letter and end with a period.
• Watch out for typos.
• Obsolete copy/pasted code hurts, make sure you update comments inside copy/pasted code blocks.
• A global comment at the very top of a file explaining what the file is about and the major types/classes/functions it contains is a good idea for quickly browsing code.
• If you are forced to employ some kind of hack in your code, and there's no way around it, then add a comment that explains the hack and why it is needed. The reviewer is going to ask for one anyway.

Asynchronous Code

A lot of code in DevTools is asynchronous, because a lot of it relies on connecting to the DevTools debugger server and getting information from there in an asynchronous fashion.

It's easy to make mistakes with asynchronous code, so here are a few guidelines that should help:

• Prefer promises over callbacks.
• Use the new Promise(() => {}) syntax.
• Don't forget to catch rejections by defining a rejection handler: promise.then(() => console.log("resolved"), () => console.log("rejected")); or promise.catch(() => console.log("rejected"));.
• Make use of Tasks and generator functions to make asynchronous code look synchronous.

React & Redux

Basic code style

You can find React-specific code style rules in the .eslintrc file.

Components

• Default to creating components as stateless function components.
• If you need local state, lifecycle methods, or to access the DOM node, you’ll need to use a class-based component instead. Make sure there is a comment explaining why you needed it.

PropTypes

• Use PropTypes to define the expected properties of your component. Each directly accessed property (or child of a property) should have a corresponding PropType.
• Use isRequired for any required properties.
• Place the propTypes definition at the top of the component. If using a stateless function component, place it above the declaration of the function.
• Where the children property is used, consider validating the children.