From dfe745c9e606ee28b903319ab5559ac736009e2b Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 4 Jul 2016 17:15:15 +0100 Subject: [PATCH 1/7] Add a JS code style doc --- code_style.rst | 116 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) create mode 100644 code_style.rst diff --git a/code_style.rst b/code_style.rst new file mode 100644 index 0000000000..ff48bc381d --- /dev/null +++ b/code_style.rst @@ -0,0 +1,116 @@ +Matrix JavaScript/ECMAScript Style Guide +======================================== + +The intention of this guide is to make Matrix's JavaScript codebase clean, +consistent with other popular JavaScript styles and consistent with the rest of +the Matrix codebase. For reference, the Matrix Python style guide can be found +at https://github.com/matrix-org/synapse/blob/master/docs/code_style.rst + +General Style +------------- + +- 4 spaces to indent, for consistency with Matrix Python. +- Max line width: 79 chars (with flexibility to overflow by a "few chars" if + the overflowing content is not semantically significant and avoids an + explosion of vertical whitespace). +- No trailing whitespace at end of lines. +- Don't indent empty lines. +- One newline at the end of the file. +- Unix newlines, never `\r` +- Indent similar to our python code: break up long lines at logical boundaries, + more than one argument on a line is OK +- Use semicolons, for consistency with node. +- UpperCamelCase for class and type names +- lowerCamelCase for functions and variables. +- Single line ternary operators are fine. +- UPPER_CAMEL_CASE for constants +- Single quotes for strings, for consistency with most JavaScript styles:: + "bad" // Bad + 'good' // Good +- Use parentheses instead of '\\' for line continuation where ever possible +- Open braces on the same line (consistent with Node):: + if (x) { + console.log("I am a fish"); // Good + } + + if (x) + { + console.log("I am a fish"); // Bad + } +- Spaces after `if`, `for`, `else` etc, no space around the condition:: + if (x) { + console.log("I am a fish"); // Good + } + + if(x) { + console.log("I am a fish"); // Bad + } + + if ( x ) { + console.log("I am a fish"); // Bad + } +- Declare one variable per var statement (consistent with Node). Unless they + are simple and closely related. If you put the next declaration on a new line, + treat yourself to another `var`:: + var key = "foo", + comparator = function(x, y) { + return x - y; + }; // Bad + + var key = "foo"; + var comparator = function(x, y) { + return x - y; + }; // Good + + var x = 0, y = 0; // Fine + + var x = 0; + var y = 0; // Also fine +- A single line `if` is fine, all others have braces. This prevents errors when adding to the code.:: + if (x) return true; // Fine + + if (x) { + return true; // Also fine + } + + if (x) + return true; // Not fine +- Terminate all multi-line lists with commas:: + var mascots = [ + "Patrick", + "Shirley", + "Colin", + "Susan", + "Sir Arthur David" // Bad + ]; + + var mascots = [ + "Patrick", + "Shirley", + "Colin", + "Susan", + "Sir Arthur David", // Good + ]; +- Use `null`, `undefined` etc consistently with node: + Boolean variables and functions should always be either true or false. Don't set it to 0 unless it's supposed to be a number. + When something is intentionally missing or removed, set it to null. + Don't set things to undefined. Reserve that value to mean "not yet set to anything." + Boolean objects are verboten. +- Use JSDoc + +ECMAScript +---------- +- Use `let` for variables and `const` for constants. This sounds obvious, but it isn't: the ES6 `const` keyword + could be used for assign-once variables, however this guide advises against doing so on the grounds that it + confuses them with constants. +- Be careful migrating files to newer syntax. + - Don't mix `require` and `import` in the same file. Either stick to the old style or change them all. + - Likewise, don't mix things like class properties and `MyClass.prototype.MY_CONSTANT = 42;` + - Be careful mixing arrow functions and regular functions, eg. if one function in a promise chain is an + arrow function, they probably all should be. +- Apart from that, newer ES features should be used whenever the author deems them to be appropriate. +- Flow annotations are welcome and encouraged. + +React +----- +- Use ES6 classes, although bear in mind a lot of code uses createClass. From 56c73b68a9abec549b03232be8dd0e3d2e3f1f9a Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 4 Jul 2016 17:23:38 +0100 Subject: [PATCH 2/7] Use markdown because the rst wasn't formatting and we use md for everything else in this repo, and in a document that talks about consistency... --- code_style.rst => code_style.md | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) rename code_style.rst => code_style.md (94%) diff --git a/code_style.rst b/code_style.md similarity index 94% rename from code_style.rst rename to code_style.md index ff48bc381d..eea23aed9c 100644 --- a/code_style.rst +++ b/code_style.md @@ -8,7 +8,6 @@ at https://github.com/matrix-org/synapse/blob/master/docs/code_style.rst General Style ------------- - - 4 spaces to indent, for consistency with Matrix Python. - Max line width: 79 chars (with flexibility to overflow by a "few chars" if the overflowing content is not semantically significant and avoids an @@ -28,7 +27,8 @@ General Style "bad" // Bad 'good' // Good - Use parentheses instead of '\\' for line continuation where ever possible -- Open braces on the same line (consistent with Node):: +- Open braces on the same line (consistent with Node): + ``` if (x) { console.log("I am a fish"); // Good } @@ -37,7 +37,9 @@ General Style { console.log("I am a fish"); // Bad } -- Spaces after `if`, `for`, `else` etc, no space around the condition:: + ``` +- Spaces after `if`, `for`, `else` etc, no space around the condition: + ``` if (x) { console.log("I am a fish"); // Good } @@ -49,9 +51,11 @@ General Style if ( x ) { console.log("I am a fish"); // Bad } + ``` - Declare one variable per var statement (consistent with Node). Unless they are simple and closely related. If you put the next declaration on a new line, - treat yourself to another `var`:: + treat yourself to another `var`: + ``` var key = "foo", comparator = function(x, y) { return x - y; @@ -66,7 +70,9 @@ General Style var x = 0; var y = 0; // Also fine -- A single line `if` is fine, all others have braces. This prevents errors when adding to the code.:: + ``` +- A single line `if` is fine, all others have braces. This prevents errors when adding to the code.: + ``` if (x) return true; // Fine if (x) { @@ -75,7 +81,9 @@ General Style if (x) return true; // Not fine -- Terminate all multi-line lists with commas:: + ``` +- Terminate all multi-line lists with commas: + ``` var mascots = [ "Patrick", "Shirley", @@ -91,6 +99,7 @@ General Style "Susan", "Sir Arthur David", // Good ]; + ``` - Use `null`, `undefined` etc consistently with node: Boolean variables and functions should always be either true or false. Don't set it to 0 unless it's supposed to be a number. When something is intentionally missing or removed, set it to null. From 04728ae03bc041a487bd22b8d943c8a983ff794b Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 7 Jul 2016 12:09:02 +0100 Subject: [PATCH 3/7] PR fixes + more general notes --- code_style.md | 38 +++++++++++++++++++++++++++++++++++--- 1 file changed, 35 insertions(+), 3 deletions(-) diff --git a/code_style.md b/code_style.md index eea23aed9c..4a42597b7a 100644 --- a/code_style.md +++ b/code_style.md @@ -6,6 +6,19 @@ consistent with other popular JavaScript styles and consistent with the rest of the Matrix codebase. For reference, the Matrix Python style guide can be found at https://github.com/matrix-org/synapse/blob/master/docs/code_style.rst +This document reflects how we would like Matrix JavaScript code to look, with +acknowledgement that a significant amount of code is written to older +standards. + +Write applications in modern ECMAScript and use a transpiler where necessary to +target older platforms. When writing library code, consider carefully whether +to write in ES5 to allow all JavaScript application to use the code directly or +writing in modern ECMAScript and using a transpile step to generate the file +that applications can then include. There are significant benefits in being +able to use modern ECMAScript, although the tooling for doing so can be awkward +for library code, especially with regard to translating source maps and line +number throgh from the original code to the final application. + General Style ------------- - 4 spaces to indent, for consistency with Matrix Python. @@ -23,10 +36,12 @@ General Style - lowerCamelCase for functions and variables. - Single line ternary operators are fine. - UPPER_CAMEL_CASE for constants -- Single quotes for strings, for consistency with most JavaScript styles:: +- Single quotes for strings by default, for consistency with most JavaScript styles: + ``` "bad" // Bad 'good' // Good -- Use parentheses instead of '\\' for line continuation where ever possible + ``` +- Use parentheses or `\`` instead of '\\' for line continuation where ever possible - Open braces on the same line (consistent with Node): ``` if (x) { @@ -82,7 +97,7 @@ General Style if (x) return true; // Not fine ``` -- Terminate all multi-line lists with commas: +- Terminate all multi-line lists with commas (if using a transpiler): ``` var mascots = [ "Patrick", @@ -103,6 +118,14 @@ General Style - Use `null`, `undefined` etc consistently with node: Boolean variables and functions should always be either true or false. Don't set it to 0 unless it's supposed to be a number. When something is intentionally missing or removed, set it to null. + If returning a boolean, type coerce: + ``` + function hasThings() { + return !!length; // bad + return new Boolean(length); // REALLY bad + return Boolean(length); // good + } + ``` Don't set things to undefined. Reserve that value to mean "not yet set to anything." Boolean objects are verboten. - Use JSDoc @@ -123,3 +146,12 @@ ECMAScript React ----- - Use ES6 classes, although bear in mind a lot of code uses createClass. +- Pull out functions in props to the class, generally as specific event handlers: + ``` + // Bad + {doStuff();}}> // Equally bad + // Better + // Best + ``` +- Think about whether your component really needs state: are you duplicating + information in component state that could be derived from the model? From afa6acc20a75705c4f27fa9a17aa22329fd01d88 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Jul 2016 15:42:42 +0100 Subject: [PATCH 4/7] All the trailing commas --- code_style.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code_style.md b/code_style.md index 4a42597b7a..c46592f244 100644 --- a/code_style.md +++ b/code_style.md @@ -97,7 +97,7 @@ General Style if (x) return true; // Not fine ``` -- Terminate all multi-line lists with commas (if using a transpiler): +- Terminate all multi-line lists, object literals, imports and ideally function calls with commas (if using a transpiler). Note that trailing function commas require explicit configuration in babel at time of writing: ``` var mascots = [ "Patrick", From 1a3bc814e1cb81f2d1d32d73ad9ae11c14b94ccd Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Jul 2016 15:58:18 +0100 Subject: [PATCH 5/7] clarify event handlers --- code_style.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code_style.md b/code_style.md index c46592f244..9078d8cdb2 100644 --- a/code_style.md +++ b/code_style.md @@ -151,7 +151,7 @@ React // Bad {doStuff();}}> // Equally bad // Better - // Best + // Best, if onFooClick would do anything other than directly calling doStuff ``` - Think about whether your component really needs state: are you duplicating information in component state that could be derived from the model? From 0fdc2d817c6c8640048baa3b4b9d6bd71b448e0a Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Jul 2016 15:59:34 +0100 Subject: [PATCH 6/7] mark as jsx --- code_style.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code_style.md b/code_style.md index 9078d8cdb2..0ef16aedf5 100644 --- a/code_style.md +++ b/code_style.md @@ -147,7 +147,7 @@ React ----- - Use ES6 classes, although bear in mind a lot of code uses createClass. - Pull out functions in props to the class, generally as specific event handlers: - ``` + ```jsx // Bad {doStuff();}}> // Equally bad // Better From ffbe045fcc645bda4fcc0c0292c973774ef1ac1a Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 11 Jul 2016 10:10:51 +0100 Subject: [PATCH 7/7] Change to const-by-default --- code_style.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/code_style.md b/code_style.md index 0ef16aedf5..6ec2597aa2 100644 --- a/code_style.md +++ b/code_style.md @@ -132,9 +132,7 @@ General Style ECMAScript ---------- -- Use `let` for variables and `const` for constants. This sounds obvious, but it isn't: the ES6 `const` keyword - could be used for assign-once variables, however this guide advises against doing so on the grounds that it - confuses them with constants. +- Use `const` unless you need a re-assignable variable. This ensures things you don't want to be re-assigned can't be. - Be careful migrating files to newer syntax. - Don't mix `require` and `import` in the same file. Either stick to the old style or change them all. - Likewise, don't mix things like class properties and `MyClass.prototype.MY_CONSTANT = 42;`