Docs: Update JavaScript guidelines to reflect current best practices #9037

aduth · 2016-10-31T19:15:55Z

This pull request seeks to update our JavaScript guidelines to reflect consistent best practices throughout.

Much of this should be non-controversial:

Updating var to let/const
Updating React.createClass to React.Component
Accounting for server-side rendering in window global usage
Adding links to external resources
Document common gotchas and lint-enforced guidelines such as 'valueOf' in object, early function returns, typeof object === 'object

Other changes may warrant debate:

Discourage undesirable behaviors such as disabling ESLint rules and complex ternary expressions by removing them from the guidelines
Defer to external resources for ES2015 usage, as our own examples are likely to fall out of date quickly, and we should encourage using all ES2015 features, not just those we document

Prefer JSDoc documentation

Slightly updated since ES5.1

Exception removed in #5829

Instead defer to external resources

Discourage disabling rules, because you’re probably wrong if you’re disabling a rule

Specific to WordPress?

Perhaps controversially, but I argue that complex ternaries are difficult to read. While I did not include any guidelines _against_ using complex ternaries, I similarly don’t think it’s advisable that we include recommendations for how to use them.

matticbot · 2016-10-31T19:15:56Z

Test live: https://calypso.live/?branch=update/js-docs

blowery · 2016-10-31T19:25:58Z

docs/coding-guidelines/javascript.md


 ## Iteration

 The functional programming inspired methods that were added in ECMA5 improve readability. Use them throughout.

 ```js
-var posts = postList.map( function( post ) {
+const posts = postList.map( function( post ) {


maybe use an arrow function here?

maybe use an arrow function here?

Done in d7b075a

dmsnell · 2016-10-31T19:28:13Z

docs/coding-guidelines/javascript.md

-    when: 4, 'you are': 15 };
-
+map = { ready: 9,
+	when: 4, 'you are': 15 };


two ideas:

use const-first thinking as an example instead

not name this map in order to prevent confusion from Array.prototype.map

two ideas:

Yeah, I went back-and-forth a few times with the const. I agree we should promote it, but I didn't want to confuse by having const for the same variable name multiple times in the same example block. I might see to using different variable names, or just creating separate code blocks.

lots of these are good vs bad, so I think it's reasonable to assume when reading that we're comparing two things that don't exist together

use const-first thinking as an example instead
not name this map in order to prevent confusion from Array.prototype.map

Improved examples, including separate code blocks with const instead of let, in ffedbac.

dmsnell · 2016-10-31T19:28:59Z

docs/coding-guidelines/javascript.md

 }

-// Unlike jQuery, WordPress prefers a space after the ! negation operator.
-// This is also done to conform to our PHP standards.


thank you for removing this!

dmsnell · 2016-10-31T19:29:26Z

docs/coding-guidelines/javascript.md

-    $( '.container' ).val( array[ i ] );
+for ( let i = 0; i < 100; i++ ) {
+	object[ array[ i ] ] = someFn( i );
+	$( '.container' ).val( array[ i ] );


oh my. I hope to never find this actual code in a review

oh my. I hope to never find this actual code in a review

Removed this specific line in 4d171dd . This could also be improved to a reduce or forEach, but I think the point is to demonstrate for spacing.

yeah I got the point of it, but was just a bit irked that we used such a nasty example. we could have used a much simpler loop body

yeah I got the point of it, but was just a bit irked that we used such a nasty example. we could have used a much simpler loop body

I'm going to leave this as-is, but d7b075a includes an emphasis on ES5+ Array methods for iteration.

dmsnell · 2016-10-31T19:29:55Z

docs/coding-guidelines/javascript.md

@@ -125,54 +123,105 @@ Indentation and line breaks add readability to complex statements.

 Tabs should be used for indentation.

+Try to return early from a function to avoid deeply tabbed functions akin to ["Callback Hell"](http://callbackhell.com/).


maybe deep indentation would be more appropriate than tabbed

maybe deep indentation would be more appropriate than tabbed

Updated in e3b41c4.

dmsnell · 2016-10-31T19:45:49Z

thanks for taking the time to update these @aduth. overall I think it would be great to place more emphasis on const and less on for loops. I'm worried that even in places where we're not using those as good examples, their presence will encourage their use. we have plenty of examples of for loops in there and appropriate questions on their style, but do we need more indicating when array operations would be better suited? we could add in a few good/bad examples of using loops vs. map, filter, forEach.

aduth · 2016-10-31T19:50:18Z

@dmsnell : We have a short section "Iteration" that we could expand to cover more use-cases of map, filter, forEach, reduce, and perhaps pointers to a few more Lodash methods.

aduth · 2016-11-01T17:03:02Z

Improved "Iteration" section with an emphasis on ES5+ Array methods in d7b075a.

dmsnell · 2016-11-01T17:26:15Z

docs/coding-guidelines/javascript.md

-map = { ready: 9,
-	when: 4, 'you are': 15 };
+const labels = { facebook: 'Facebook',
+	twitter: 'Twitter', 'google-plus': 'Google Plus' };


haha, I almost made a comment like, "what are you thinking?" and then I noticed this is the "bad" example 😉

haha, I almost made a comment like, "what are you thinking?" and then I noticed this is the "bad" example 😉

Yeah, I rearranged all of the examples to show "Bad" first.

dmsnell · 2016-11-01T17:27:47Z

docs/coding-guidelines/javascript.md

+```js
+// After
+const sumLabel = `The sum of ${ a } and ${ b } plus ${ c } is ${ a + b + c }`;
+```


w00t! I ❤️ this!

w00t! I ❤️ this!

Thanks! As I was changing the original in "Multi-line Statements" to use const, I felt depressed that we'd concatenate a string like that, so I thought it was a nice progression to see how the same example can be improved further using template literals.

dmsnell · 2016-11-01T17:37:38Z

docs/coding-guidelines/javascript.md

@@ -420,6 +402,18 @@ const myStr = "You're amazing just the way you are.";
 const component = <div className="post"></div>;
 ```

+[ES2015 template literals](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals) are available as an alternative to string concatenation when including variables in a string.


There is this obscure usage of tag functions with template literals. Do we want to acknowledge them and make some styling decisions?

function tag(strings, ...values) { return { blog: values[0], user: values[1] } } const s = tag`Make blog ${ 'catbacon' } with user ${ 'Porkat' }`; // s === { // blog: 'cat bacon', // user: 'Porkat' // }

There is this obscure usage of tag functions with template literals.

I personally feel we should avoid advocating their usage. That is... unless we ever adopt Relay 😉 (reference)

I couldn't agree more (about not using them)

youknowriad · 2016-11-02T10:09:24Z

docs/coding-guidelines/javascript.md


- [Rest](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/rest_parameters) at MDN
+- Use [stateless function components or the `React.Component` class](https://facebook.github.io/react/docs/components-and-props.html#functional-and-class-components) instead of `React.createClass`


What do we do about mixins? Are we encouraged to replace them by HoC when we think It's appropriate?
For example, LinkedStateMixin and dirtyLinkedState can be replaced nicely with a HoC, I think.

What's our current approach with that? Maybe we could add a note about mixins on this doc.

We definitely want to avoid all mixins, and LinkedStateMixin has been on the black list for a long time. We have various ways to remove them: higher order components for sure, but also for some things local component state or proper use of our Redux state/query components

gwwar · 2016-11-02T22:00:55Z

docs/coding-guidelines/javascript.md

 ## Strings

 Use single-quotes for string literals:

 ```js
-var myStr = 'strings should be contained in single quotes';
+const myStr = 'strings should be contained in single quotes';
 ```

 When a string contains single quotes, they need to be escaped with a backslash (\\):

 Double quotes can be used in cases where there is a single quote in the string or in JSX attributes.


Do we not have a style preference one way or another for that? I personally think the latter is much more readable.

'The Cat\'s Hat'

vs

"The Cat's Hat"

I personally agree with your preference. If there's enough in agreement, I'd be okay with removing the allowed double-quote exception.

I'm confused here... I read @gwwar's comment as preferring "The Cat's Hat"...

@blowery : Oh, you're right, I misread it. In which case then... I disagree 😄

I definitely prefer "The Cat's Hat" over 'The Cat\'s Hat' and I also prefer (backtick)The Cat's Hat(backtick) over it too.

Why escape things when we don't have too? Just gets in the way

I'm with @gwwar

Why escape things when we don't have too?

Consistency for one. Also, what happens when the string is updated and no longer needs escaping? Do we change the " back to '? This seems easy to overlook and is unnecessary effort.

Consistency seems subjective here. What are we being consistent about: quote types or escaping quotes?

If we're consistent about quote types then we're basically requiring that we never use double-quotes, except for with JSX.

If we're consistent about escaping, we're saying never escape quotes (except maybe the back-tick inside of template literals).

In this case I'm willing to sacrifice a small consistency for the sake of providing normal-reading sentences.

gwwar · 2016-11-02T22:10:58Z

👍 Thanks for updating this @aduth !

spen · 2016-11-04T21:58:33Z

docs/coding-guidelines/javascript.md

+// Bad
+const sumLabel = 'The sum of ' + a + ' and ' + b + ' plus ' + c
+	+ ' is ' + ( a + b + c );
+```js


Just a heads up, looks like a couple of extraneous '```js's got in - causes the output to look like:

// Bad const sumLabel = 'The sum of ' + a + ' and ' + b + ' plus ' + c + ' is ' + ( a + b + c ); ```js ```js // Good const sumLabel = 'The sum of ' + a + ' and ' + b + ' plus ' + c + ' is ' + ( a + b + c );

👍 good catch!

Good catch indeed, fixed in cb8cf91

gwwar · 2016-11-08T18:34:15Z

Unless folks have found any blockers, I think this is a great improvement, and we should

aduth · 2016-11-09T15:03:17Z

Unless folks have found any blockers, I think this is a great improvement, and we should

I'm sure there's still room for improvement, but we can always revisit it in a future pull request. Hopefully nothing changed here was too controversial.

Seems a few ideas to consider for a future pull request are:

Double-quoted strings when containing apostrophe
Multi-line conditions indentation
Reevaluate == null strict equality exception (my own desire)

In the meantime, I'm going to squash and merge this one.

…9037) * Docs: Update var to let/const in JS guidelines * Docs: Remove inline comments on parameter list Prefer JSDoc documentation * Docs: Add caveat about typeof object * Docs: Update truthy link to ES2015 specification Slightly updated since ES5.1 * Docs: Add explanation for boolean return value function naming * Docs: Fix object key spacing per guidelines Exception removed in #5829 * Docs: Add link for Yoda conditions reference * Docs: Improve React component best practices * Docs: Remove ES6 usage examples Instead defer to external resources * Docs: Add reference links for ESLint configuration Discourage disabling rules, because you’re probably wrong if you’re disabling a rule * Docs: Add note about EditorConfig * Docs: Remove jQuery clarification Specific to WordPress? * Docs: Add early return example * Docs: Add else return example * Docs: Remove complex ternary example Perhaps controversially, but I argue that complex ternaries are difficult to read. While I did not include any guidelines _against_ using complex ternaries, I similarly don’t think it’s advisable that we include recommendations for how to use them. * Docs: Add window typeof check tip for SSR * Docs: Add caveat about object "in" operator * Docs: Improve clarity of `else` `return` guideline * Docs: Use tabs consistently in JS docs * Docs: Promote discussing lint rules * Docs: Use consistent capitalization in comments * Docs: Deeply tabbed -> deep indentation * Docs: Remove jQuery specific spacing example * Docs: Use arrow functions where applicable * Docs: Add link for automatic semicolon insertion * Docs: Improve examples naming, logic, ordering * Docs: Emphasize ES5+ Array methods for iteration * Docs: Include template literal example * Docs: Fix closing code block syntax

aduth added 17 commits October 31, 2016 15:06

Docs: Update var to let/const in JS guidelines

182864f

Docs: Remove inline comments on parameter list

7ebc01e

Prefer JSDoc documentation

Docs: Add caveat about typeof object

ab72e88

Docs: Update truthy link to ES2015 specification

8f09a83

Slightly updated since ES5.1

Docs: Add explanation for boolean return value function naming

8afd673

Docs: Fix object key spacing per guidelines

f258519

Exception removed in #5829

Docs: Add link for Yoda conditions reference

6ec38c6

Docs: Improve React component best practices

60a0f12

Docs: Remove ES6 usage examples

e80800a

Instead defer to external resources

Docs: Add reference links for ESLint configuration

4b26ff5

Discourage disabling rules, because you’re probably wrong if you’re disabling a rule

Docs: Add note about EditorConfig

38f85ea

Docs: Remove jQuery clarification

1dc7fcd

Specific to WordPress?

Docs: Add early return example

544aac6

Docs: Add else return example

be210dd

Docs: Add window typeof check tip for SSR

8874a07

Docs: Add caveat about object "in" operator

519dfa4

aduth added Documentation [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 31, 2016

aduth added 2 commits October 31, 2016 15:19

Docs: Improve clarity of else return guideline

04bd011

Docs: Use tabs consistently in JS docs

3dfa55e

blowery reviewed Oct 31, 2016

View reviewed changes

dmsnell reviewed Oct 31, 2016

View reviewed changes

aduth added 3 commits October 31, 2016 15:30

Docs: Promote discussing lint rules

8eaf5b4

Docs: Use consistent capitalization in comments

818cdf8

Docs: Deeply tabbed -> deep indentation

e3b41c4

aduth added 5 commits November 1, 2016 12:07

Docs: Use arrow functions where applicable

30f204b

Docs: Add link for automatic semicolon insertion

a50cb57

Docs: Improve examples naming, logic, ordering

ffedbac

Docs: Emphasize ES5+ Array methods for iteration

d7b075a

Docs: Include template literal example

801d151

dmsnell reviewed Nov 1, 2016

View reviewed changes

youknowriad reviewed Nov 2, 2016

View reviewed changes

gwwar reviewed Nov 2, 2016

View reviewed changes

gwwar mentioned this pull request Nov 4, 2016

Me: Add EditGravatar to profile under feature flag #9156

Merged

spen suggested changes Nov 4, 2016

View reviewed changes

Docs: Fix closing code block syntax

cb8cf91

aduth mentioned this pull request Nov 8, 2016

Sharing: Fix warnings on the Sharing Buttons Page #9219

Merged

gwwar mentioned this pull request Nov 8, 2016

Added unit test for select-dropdown component #9150

Merged

aduth added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 9, 2016

aduth merged commit 1de954c into master Nov 9, 2016

aduth deleted the update/js-docs branch November 9, 2016 15:03

blowery removed the [Status] Ready to Merge label Nov 10, 2016

spen mentioned this pull request Nov 12, 2016

Docs: Javascript style guide - multi-line arrow functions are okay without braces #9203

Closed

gwwar mentioned this pull request Feb 28, 2017

Directly: Add library that wraps around standard API #11489

Merged

		@@ -125,54 +123,105 @@ Indentation and line breaks add readability to complex statements.

		Tabs should be used for indentation.

		Try to return early from a function to avoid deeply tabbed functions akin to ["Callback Hell"](http://callbackhell.com/).


		- [Rest](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/rest_parameters) at MDN
		- Use [stateless function components or the `React.Component` class](https://facebook.github.io/react/docs/components-and-props.html#functional-and-class-components) instead of `React.createClass`

Docs: Update JavaScript guidelines to reflect current best practices #9037

Docs: Update JavaScript guidelines to reflect current best practices #9037

Conversation

aduth commented Oct 31, 2016

matticbot commented Oct 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Oct 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell commented Oct 31, 2016

aduth commented Oct 31, 2016

aduth commented Nov 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Nov 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

gwwar commented Nov 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwwar commented Nov 8, 2016

aduth commented Nov 9, 2016

aduth Oct 31, 2016 •

edited

Loading

aduth Nov 1, 2016 •

edited

Loading

dmsnell Nov 4, 2016 •

edited

Loading