Use `standard` JavaScript Style #4909

zeke · 2016-03-24T20:53:33Z

This pull request migrates all of electron's JavaScript code to standard style.

Why use standard on a project like electron? The standard README summarizes it well:

Adopting standard style means ranking the importance of code clarity and community conventions higher than personal style. This might not make sense for 100% of projects and development cultures, however open source can be a hostile place for newbies. Setting up clear, automated contributor expectations makes a project healthier.

The Rules

2 spaces – for indentation
Single quotes for strings – except to avoid escaping
No unused variables – this one catches tons of bugs!
No semicolons – It's fine. Really!
Never start a line with ( or [
- This is the only gotcha with omitting semicolons – automatically checked for you!
- More details
Space after keywords if (condition) { ... }
Space after function name function name (arg) { ... }
Always use === instead of == – but obj == null is allowed to check null || undefined.
Always handle the node.js err function parameter
Always prefix browser globals with window – except document and navigator are okay
- Prevents accidental use of poorly-named browser globals like open, length,
  event, and name.

For more details, see https://github.com/feross/standard/blob/master/RULES.md

The Refactor

I kicked off the refactor using standard-format to automatically convert much of the code. This got me a long way, but the tool has its share of issues. Sometimes it chokes on files containing valid syntax, and sometimes it overlooks files that contain invalid syntax. ¯\_(ツ)_/¯

After squeezing everything I could out of standard-format, I removed it from the dependencies because we probably won't need automated code refactoring once this refactor rolls out.

Next I installed snazzy, a wrapper around standard that produces glamorous and colorful output. This was nice because it made the process of manual code cleanup much more... pleasant. As I made changes, I committed them often to ensure the test suite continued to pass.

Once I got all the code cleaned up, I uninstalled snazzy in favor of vanilla standard for a few reasons:

standard is more actively maintained than snazzy
standard is a more recognizable name for the dependency
snazzy's output is a little buggy sometimes

The Most Common Fixes

Here's a sorted list of the most common errors, generated by standard-summary:

5966   Extra semicolon
1615   Missing space before function parentheses
380    'it' is not defined
318    Strings must use singlequote
206    'describe' is not defined
55     Return statement should not contain assignment
52     '$' is not defined
38     Expected { after 'if' condition
26     'afterEach' is not defined
24     Expected '===' and instead saw '=='
23     Missing '()' invoking a constructor
17     Block must not be padded by blank lines
16     Unexpected trailing comma
13     Multiple spaces found before '+='
12     The '__proto__' property is deprecated
10     Expected space(s) after "catch"
9      Multiple spaces found before '='
8      Expected '!==' and instead saw '!='
8      Gratuitous parentheses around expression
7      More than 1 blank line not allowed
7      Expected { after 'else'
6      Unexpected use of comma operator
5      Use path.join() or path.resolve() instead of + to create paths
5      Do not use 'new' for side effects
3      Infix operators must be spaced
...

Globals

The test suite contains a number of global variables, mostly from mocha: after, afterEach, before, beforeEach, Blob, btoa, describe, DevToolsAPI, Event, fetch, HTMLElement, HTMLObjectElement, InspectorFrontendHost, it, location, MutationObserver, Response, self, SharedWorker, URL, WebInspector, WebSocket, WebView, Worker, xdescribe, xit.

These globals are whitelisted in a few different ways:

a standard.globals array in package.json is used for the most common repeat offenders, like mocha and jquery.
a /* globals foo, bar */ comment at the top of the offending file, for less commonly used globals.

For more info, see the standard docs about globals

{
  "standard": {
    "globals": ["$", "after", "afterEach", "before", "beforeEach", "describe", "it", "location"]
  }
}

Ignoring Stuff

In package.json, there's an ignore array that accepts glob patterns for files and directories:

{
  "standard": {
    "ignore": ["/vendor"],
  }
}

To ignore a few lines of code, wrap them in these comments:

/* eslint-disable */
// wild and crazy javascript goes in here!
/* eslint-enable */

To ignore a single line of code, use a // eslint-disable-line comment:

 let obj = new (Function.prototype.bind.apply(constructor, [null].concat(args))) // eslint-disable-line

CoffeeScript Legacy

Back in January, the electron codebase was converted from Coffeescript to JavaScript. This automated refactor left behind a number of code artifacts, some of which have been cleaned up here.

All implicit returns that were doing assignment have been removed (e.g. return foo = 1). There are undoubtedly still lots of unneeded (yet syntactically valid) implicit returns left over.

There's still plenty of leftover cruft from the coffeescript conversion, but most of it is harmless. Any time you see ref = in a javascript file, it's pretty safe to assume that it's old coffee code that could use some cleanup.

Getting and Setting Prototype

// old setter
thing.__proto__ = EventEmitter.prototype

// new setter
Object.setPrototypeOf(thing, EventEmitter.prototype)

// old getter
thing.__proto__

// new getter
Object.getPrototypeOf(thing)

Invoking Constructors

It works to leave off the parens, but don't do it:

// no
new Thing

// yes
new Thing()

Merging this PR

As @paulcbetts recently observed:

On a more important tip, for older projects, try to get as many PRs as possible merged before you make the conversion, because the rest of the PRs will get rekt by merge conflicts

As of this writing, there are 15 open pull requests. We should decide which of those we want to land before this refactor, and which we should close or rebase post-standard.

Given the haphazard and compulsively granular commit history in this branch, I'd be happy to squash it all into a single commit if that's preferred by the team.

ungoldman · 2016-03-29T00:10:03Z

Woo cool!

zeke · 2016-03-29T18:09:00Z

This is ready for review!

@jlord @kevinsawicki @zcbenz @simurai

anaisbetts · 2016-03-29T18:45:31Z

@zeke You're doin' the Lord's work, 👍

kevinsawicki · 2016-03-30T02:43:17Z

The test suite contains a number of global variables, mostly from mocha

Is it possible to configure the mocha globals as only available inside the spec/ folder? So that using it in a spec doesn't throw a standard error, but doing so in lib/ would.

kevinsawicki · 2016-03-30T02:44:00Z

package.json

@@ -4,16 +4,32 @@
  "devDependencies": {
    "asar": "^0.10.0",
    "eslint": "^2.1.0",


Could this now be removed?

anaisbetts · 2016-03-30T02:45:39Z

Is it possible to configure the mocha globals as only available inside the spec/ folder? So that using it in a spec doesn't throw a standard error, but doing so in lib/ would.

Just include a .eslintrc in that folder with mocha: true in the env

kevinsawicki · 2016-03-30T02:47:07Z

To ignore a few lines of code, wrap them in these comments:

@zeke do you think it is possible to avoid these completely in this PR? It would seem ideal to not have any pollution of standard/eslint comments like previously.

zeke · 2016-03-30T03:16:38Z

Just include a .eslintrc in that folder with mocha: true in the env

@paulcbetts are you sure that works with standard? I just created an rc file per the eslint.org docs, but it doesn't seem to be respected. Am I doing this right?

❯ cat spec/.eslintrc.js 
module.exports = {
  env: {
    mocha: true
  }
}

~/electron/electron standard*
❯ standard | head -5 

spec/api-app-spec.js:9:1: 'describe' is not defined.
spec/api-app-spec.js:10:3: 'it' is not defined.
spec/api-app-spec.js:14:3: 'it' is not defined.
spec/api-app-spec.js:28:1: 'describe' is not defined.
spec/api-app-spec.js:29:3: 'describe' is not defined.

It would seem ideal to not have any pollution of standard/eslint comments

@kevinsawicki I agree. There were just a handful of spots in the code where I couldn't easily figure out how to refactor it without breaking the test suite. Perhaps we can pair tomorrow and try to knock them out?

kevinsawicki · 2016-03-30T03:23:02Z

Perhaps we can pair tomorrow and try to knock them out?

Sounds great 👍 ✖️ 💯

kevinsawicki · 2016-03-30T03:24:22Z

are you sure that works with standard?

Yeah, I wasn't clear if it did either.

Since there is a spec/package.json, perhaps the globals config could go directly in there?

anaisbetts · 2016-03-30T03:26:08Z

spec/.eslintrc.js

Edit: Hm, I have no idea if it works with standard, just copy-paste their eslintrc if it doesn't

zeke · 2016-03-30T03:35:09Z

@paulcbetts:

It doesn't end with .js, just .eslintrc

I actually tried that format first, then tried the javascript format described in the eslint.org docs:

JavaScript - use .eslintrc.js and export an object containing your configuration.

@kevinsawicki:

Since there is a spec/package.json, perhaps the globals config could go directly in there?

Good idea, but it didn't work. Perhaps @feross can give us some pointers?

feross · 2016-03-30T07:10:19Z

There's currently no way use .eslintrc files in subfolders to override the default rule set. This was an intentional design decision. I'm sorry that it's causing trouble for you in this situtation 😢

If setting mocha: true at the top-level is not acceptable, here's an alternate solution:

Add your globals to spec/package.json, change the top-level package.json script to ignore spec/ and run standard separately for the spec/ folder, like this:

{
  "lint": "standard && standard spec && python ./script/cpplint.py",
}

zeke · 2016-03-30T20:23:14Z

I tested the standard && standard spec approach and it works. It's implemented in a separate branch. Here's the commit: 525f3cc

My personal preference would be to keep all the config in the top-level package.json for the sake of discoverability/simplicity, and for the composability of running one command to lint all the javascript, instead of two. See https://github.com/atom/electron/blob/e342d65abb9d4df85fd508b55669b9eaf2c77681/package.json#L13-L24

The difference are subtle though. Either approach works for me.

zeke · 2016-03-30T22:56:27Z

@kevinsawicki and I paired on this for a bit today. Here's what we got done:

Removed all but one eslint style exception. (eval)
Split out the standard configuration into /package.json and /spec/package.json
Removed all inline /* globals ... */ comments in favor of standard.env declarations in /spec/package.json
Got the CI builds passing! (Except for Windows 64, which timed out; Kevin says it's flaky)

Kevin said he'll review the outstanding pull requests and merge any that seem like they need to land first. Barring any other feedback or concerns from the rest of the team, this is ready to 🐑!

kevinsawicki · 2016-03-30T23:00:26Z

lib/renderer/override.js

-})();
+var _isVisible = true
+var _isMinimized = false
+;(function () {


Is this leading ; required?

'tis. We could name the function, then invoke it.

We could name the function, then invoke it.

I'm in favor of doing this ☝️

Done. 8a159de

standard-format freaks out on this line: let obj = new (Function.prototype.bind.apply(constructor, [null].concat(args))) http://stackoverflow.com/questions/1606797/use-of-apply-with-new-operato r-is-this-possible

kevinsawicki · 2016-03-31T00:51:22Z

Kevin said he'll review the outstanding pull requests and merge any that seem like they need to land first.

I landed one and rebased this branch, no plans to merge any others before this lands.

zcbenz · 2016-04-01T02:13:49Z

Let's merge this so we can merge other PRs.

ichpuchtli · 2016-04-01T05:32:03Z

RIP git blame functionality

zcbenz · 2016-04-01T05:34:34Z

We have lost him long time ago when converting coffee to js.

bbondy · 2016-04-01T13:43:42Z

@ichpuchtli you can git blame more than one level deep.
@zeke nice work!

joeytwiddle · 2016-04-07T06:25:50Z

I was curious what @bbondy meant regard multi-level git blame.

So far I found git log -L, git gui, the recursive-blame package, and git log -S.

zeke force-pushed the standard branch from eabbebd to e3d56da Compare March 25, 2016 20:12

zeke mentioned this pull request Mar 29, 2016

add standard-summary to readme standard/standard#477

Merged

zeke force-pushed the standard branch from ba6ebc7 to 1c87d03 Compare March 29, 2016 17:33

zeke changed the title ~~[WIP] Standard~~ Use standard JavaScript Style Mar 29, 2016

kevinsawicki reviewed Mar 30, 2016
View reviewed changes

package.json

@@ -4,16 +4,32 @@

"devDependencies": {

"asar": "^0.10.0",

"eslint": "^2.1.0",

Copy link

Contributor

kevinsawicki Mar 30, 2016

Choose a reason for hiding this comment

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

Could this now be removed?

zeke force-pushed the standard branch from 25971cd to e0a3aa0 Compare March 30, 2016 22:04

kevinsawicki reviewed Mar 30, 2016
View reviewed changes

zeke added 9 commits March 30, 2016 17:00

add standard-format to devDeps

6e31e51

mention standard in coding style doc

f25c3d3

first pass at standardizing; suite still passing!

4794385

manually standardize this tricky file

4d4f479

manual syntax fixes on rpc server

ee18129

tiptoeing along, keeping the suite passing

ca7b492

do not standardize rpc-server.js because reasons

f61010d

standard-format freaks out on this line: let obj = new (Function.prototype.bind.apply(constructor, [null].concat(args))) http://stackoverflow.com/questions/1606797/use-of-apply-with-new-operato r-is-this-possible

finish standardizing crash reporter

80f7c82

autoformat more easy files

c845ea8

zeke added 19 commits March 30, 2016 17:00

standardize by hand

2c3cacd

replace __proto__ with Object.setPrototype

4e2f131

finish standardizing!

42e7ee2

replace snazzy with standard; autoformat package.json

98b4353

do not return and assign

6f84537

replace eslint with standard

e156fae

update CONTRIBUTING doc

71f8ba6

use standard.env.mocha instead of a list of globals

0fd0887

remove eslint from devDependencies

1a18151

avoid using eslint comment exceptions

b5afad9

restrict mocha scope to /spec; break out link tasks

6425ef4

use window.location instead of location

47a61e9

try to fix CI linting

afbc914

say we are linting

0d11b75

do not lint on windows CI

aadc0be

eradicate all per-file eslint globals

7c58f7f

derp

d5e8bb7

fix a minor style issue

9efd29d

give anonymous function a name

252121a

kevinsawicki force-pushed the standard branch from 8a159de to 252121a Compare March 31, 2016 00:00

zcbenz merged commit 7023528 into master Apr 1, 2016

zcbenz deleted the standard branch April 1, 2016 02:13

Empact mentioned this pull request Apr 13, 2016

Apply standard js formatting brave/muon#16

Closed

zeke mentioned this pull request Apr 22, 2016

standardify jdx/npm-register#11

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `standard` JavaScript Style #4909

Use `standard` JavaScript Style #4909

zeke commented Mar 24, 2016

ungoldman commented Mar 29, 2016

zeke commented Mar 29, 2016

anaisbetts commented Mar 29, 2016

kevinsawicki commented Mar 30, 2016

kevinsawicki Mar 30, 2016

anaisbetts commented Mar 30, 2016

kevinsawicki commented Mar 30, 2016

zeke commented Mar 30, 2016

kevinsawicki commented Mar 30, 2016

kevinsawicki commented Mar 30, 2016

anaisbetts commented Mar 30, 2016

zeke commented Mar 30, 2016

feross commented Mar 30, 2016

zeke commented Mar 30, 2016

zeke commented Mar 30, 2016

kevinsawicki Mar 30, 2016

zeke Mar 30, 2016

jlord Mar 30, 2016

zeke Mar 30, 2016

kevinsawicki commented Mar 31, 2016

zcbenz commented Apr 1, 2016

ichpuchtli commented Apr 1, 2016

zcbenz commented Apr 1, 2016

bbondy commented Apr 1, 2016

joeytwiddle commented Apr 7, 2016

Use standard JavaScript Style #4909

Use standard JavaScript Style #4909

Conversation

zeke commented Mar 24, 2016

The Rules

The Refactor

The Most Common Fixes

Globals

Ignoring Stuff

CoffeeScript Legacy

Getting and Setting Prototype

Invoking Constructors

Merging this PR

ungoldman commented Mar 29, 2016

zeke commented Mar 29, 2016

anaisbetts commented Mar 29, 2016

kevinsawicki commented Mar 30, 2016

kevinsawicki Mar 30, 2016

Choose a reason for hiding this comment

anaisbetts commented Mar 30, 2016

kevinsawicki commented Mar 30, 2016

zeke commented Mar 30, 2016

kevinsawicki commented Mar 30, 2016

kevinsawicki commented Mar 30, 2016

anaisbetts commented Mar 30, 2016

zeke commented Mar 30, 2016

feross commented Mar 30, 2016

zeke commented Mar 30, 2016

zeke commented Mar 30, 2016

kevinsawicki Mar 30, 2016

Choose a reason for hiding this comment

zeke Mar 30, 2016

Choose a reason for hiding this comment

jlord Mar 30, 2016

Choose a reason for hiding this comment

zeke Mar 30, 2016

Choose a reason for hiding this comment

kevinsawicki commented Mar 31, 2016

zcbenz commented Apr 1, 2016

ichpuchtli commented Apr 1, 2016

zcbenz commented Apr 1, 2016

bbondy commented Apr 1, 2016

joeytwiddle commented Apr 7, 2016

Use `standard` JavaScript Style #4909

Use `standard` JavaScript Style #4909