chore: First pass adding ESLint. #7

ntwb · 2017-06-28T16:07:11Z

Includes a package.json and .gitignore

youknowriad · 2017-06-28T16:10:45Z

package.json

+    "lerna": "^2.0.0-rc.5",
+    "mkdirp": "^0.5.1"
+  },
+  "eslintConfig": {


Curious why you included the eslint config here instead of a .eslintrc. More precisely, I wonder if IDEs can read the config from here?

ntwb · 2017-06-28T16:13:40Z

In this PR I've pinned eslint-plugin-wordpress master branch, it has a larger configuration set of ESLint rules than the current eslint-config-wordpress has (eventually they will have parity, just not today)

p.s. @youknowriad I've included the .gitignore and most of package.json from #1 in the faint hope that it will aid the upcoming merge conflict 😝

youknowriad · 2017-06-28T16:14:04Z

package.json

+    "babel-preset-env": "^1.5.2",
+    "chalk": "^1.1.3",
+    "eslint": "~4.1.0",
+    "eslint-plugin-wordpress": "git://github.com/WordPress-Coding-Standards/eslint-plugin-wordpress.git#d8abe35765088dcf138d8698cf5b0c4428d575cc",


Can we publish this package to npm instead?
As a follow-up task, we could move this package inside this repository (Jest has an eslint-jest-plugin package built and used inside the repository itself)

It is published to npm, it's just out of date, there's a couple more outstanding issues to be resolved before I push a new release to npm.

The package is part of the WordPress Coding Standards GitHub Organization

Inlcudes a `package.json` and `.gitignore`

youknowriad · 2017-06-29T16:47:31Z

@ntwb I took the liberty to rebase and fix some linting issues (not all of them).

Since we're starting a new way to do JS for WordPress with this repository, I think we're in a good position to discuss these linting rules and maybe update them (only for this repository now and maybe Core later).

Here are the rules I'd like us to reconsider for this repository:

one-var grouping all variable declarations with a single var makes code unreadable IMO specially with ES6 since we can mix let and const instead I'd suggest using const as the default way of declaring variables and let if we need to mutate those.
spacing in parens, we should be consistent and always require a space before an opening/closing parens (or bracket). The current rule is obscure.
object-rest-spread is not supported by the current eslint config, we should use the babel-parser.

I'm sure I will discover more issues as I add code but these are the ones I'd like us to reconsider now. Just noting we've already forked the rules this way in Gutenberg.

youknowriad · 2017-06-29T16:51:00Z

(Looks like travis is not running npm test as expected since the tests are supposed to be failing)

ntwb · 2017-06-30T08:43:40Z

The next #core-js chat scheduled for Tuesday has coding standards on the agenda, I think we should discuss this then rather than here in the ticket as a way to get everyone on the same page.

• I'm +1 on the one-var

• The "spacing in parens" consistency is a legacy decision I believe to match/mimic the PHP coding standards, will need to do a bit of research on this one and then raise questions for both the PHP & JS in regard to changing the standard.

• Babel, I've avoided Babel, getting the current ESLint shared plugin and config up and running against what JavaScript WordPress currently uses has been my focus, so another topic for the upcoming #core-cs chat.

gziolo · 2017-07-02T03:14:04Z

Have you considered using prettier? It doesn't play nicely with all additionsl spaces that are used in WordPress coding conventions, but otherwise helps to keep one style of code formatting across all files. It would be highly helpful in the context of WordPress.

ntwb · 2017-07-02T03:53:46Z

@gziolo Thanks, yes, the use of Prettier is currently being considered

I plan on bringing this up for further discussion in this coming weeks #core-js Slack chat

gziolo · 2017-07-02T06:04:33Z

I plan on bringing this up for further discussion in this coming weeks #core-js Slack chat

That would be awesome :) We were considering using prettier at Automattic with Calypso, related discussion: Automattic/wp-calypso#12260. We put it on hold, because we didn't want to deviate from WordPress JS conventions. @samouri forked prettier at some point (https://github.com/Automattic/calypso-prettier) and he added a few tweaks to enforce spaces around parenthesis. Some people use it with CLI or IDE to simplify formatting, but to use it on a larger scale we would have to update conventions or convince Prettier maintainers to add flag for spaces around parenthesis :)

rmccue · 2017-07-02T06:07:07Z

Prettier is not terribly hard to implement: it's just as AST-to-code converter. I have work underway to implement a version of it for WP coding standards (although it may be worth seeing what the Calypso team has there).

That said, it may be worth adopting a wider coding standard. This would deviate somewhat significantly from WP though, so we should discuss it first.

gziolo · 2017-07-02T06:24:12Z

It might be worth reaching out to Prettier team and ask if we they would be fine to enable flag for spaces in parentheses. It would be much easier to contribute there than to maintain yet another tool. Given the number of WordPress installations, they should at least seriously consider such change.

ntwb · 2017-07-02T07:33:26Z

In a somewhat related manner, Prettier now includes support for CSS, the team over at stylelint are considering creating some prettier printers for formatting stylistic rules, in particular whitespace, so Prettier will inherit and print the CSS per the applicable stylelint whitespace rule configurations:

stylelint/stylelint#2532 (comment)

The new class of tools we are referring to are the pretty-printers. A couple of examples could be:

A css-pretty-printer would be able to format, based on the maximum line length, the whitespace of standard CSS constructs like at-rules, rules, declarations, functions.

A precss/scss-pretty-printer would also be able to format the whitespace of non-standard constructs like maps and nested properties. These are parsed by PostCSS as declaration and rule nodes, respectively. As well as format the whitespace of things like Control Directives & Expressions.

sugarss/sass-pretty-printer would respect SugarSS's restrictions around how to layout multiline constructs and be aware there are no curly braces.

And so on...

It's a long discussion there over the past few months (including Prettier's author). I think many of the concepts discussed there would work the same for JS with ESLint, to have whitespace around parenthesis we create a custom prettier printer such as wordpress-pretty-printer

It might be worth reaching out to Prettier team and ask if we they would be fine to enable flag for spaces in parentheses.

If creating a wordpress-pretty-printer isn't the right way forward for this then based on this comment from Prettier's author would suggest it shouldn't be too big an issue to add an option for spaces in parentheses.

p.s. I didn't want to ping said Prettier author here, we should create a P2 post or a new issue to discuss this rather than hijack this issue 😏

rmccue · 2017-07-02T07:55:33Z

(FWIW, I think it's possible to use prettier almost as-is, and simply insert an extra step that alters the intermediate representation ("doc"). Something I plan on exploring in any case.)

…eslintrc.json` file.

gziolo · 2017-07-03T14:19:20Z

If creating a wordpress-pretty-printer isn't the right way forward for this then based on this comment from Prettier's author would suggest it shouldn't be too big an issue to add an option for spaces in parentheses.

I'm fine with both approaches as long as they allow us to make Prettier default for WordPress JS code :) It would be nice if Prettier would be pluggable as much as Eslint is, then we could pipe our own transforms and make sure max-length setting is applied as a last one and takes into account additional spaces. It may be a challenge to setup it properly with additional spaces. Imagine a use case where you have max lenght 80 chars, and line is properly formatted with Prettier and contains exactly 80 chars. We run wordpress-pretty-printer and it adds additional spaces to satisfy requirements and it breaks max length rule. So we would need to perform another run which would split line properly taking into account those newly added spaces. I'm not sure if I explain it properly, but this is what we struggle with when using Prettier fork that uses style closer to WordPress world.

p.s. I didn't want to ping said Prettier author here, we should create a P2 post or a new issue to discuss this rather than hijack this issue 😏

Yes, good idea. We should definitely ping him when we have some proposal how to tackle prettier integration.

youknowriad · 2017-07-03T14:25:07Z

Just a small note on prettier. Actually, I'd have preferred if prettier didn't have any option and I encourage using it without options at all. As noted above may be it's worth considering wider coding standards :)

gziolo · 2017-07-05T09:19:04Z

.eslintrc.json

+{
+	"env": {
+		"es6": true,
+		"jest": true,


We can also add eslint-plugin-jest setup. You can check here, how I did it for Gutenberg: https://github.com/WordPress/gutenberg/pull/1382/files#diff-df39304d828831c44a2b9f38cd45289cR8.

Here we wouldn't need to turn off jest/valid-expect, because tests don't use chai.

Will push up ESLint for Jest in the morning, I ran out of time today because real life got in the way of coding 😝

nylen · 2017-07-20T13:24:48Z

packages/url/src/index.js

@@ -17,5 +17,5 @@ export function addQueryArgs( url, args ) {
 	const query = { ...parsedURL.query, ...args };
 	delete parsedURL.search;

-	return format( { ...parsedURL, query } );
+	return format({ ...parsedURL, query });


Please, no :(

nylen

I'd like to see us standardize on something closer to the Gutenberg lint config. It's time for these standards to get a major refresh - the rules around things like someFunction( arg1, {}) are just nonsensical.

nylen · 2017-07-20T13:24:51Z

packages/url/src/test/index.test.js

-	} );
-} );
+	});
+});


Please, no :(

nylen · 2017-07-20T13:25:25Z

scripts/build.js

 	const destPath = getBuildPath( file, buildDir );
-	const babelOptions = babelConfigs[ environment ];
+	const babelOptions = babelConfigs[environment];


nylen · 2017-07-20T13:25:39Z

scripts/build.js

@@ -137,7 +138,7 @@ function buildFileFor( file, silent, environment ) {
 function buildPackage( packagePath ) {
 	const srcDir = path.resolve( packagePath, SRC_DIR );
 	const pattern = path.resolve( srcDir, '**/*' );
-	const files = glob.sync( pattern, { nodir: true } );
+	const files = glob.sync( pattern, { nodir: true });


nylen · 2017-07-20T13:25:54Z

scripts/build.js

@@ -34,24 +34,25 @@ const babelDefaultConfig = JSON.parse(
 	fs.readFileSync( path.resolve( __dirname, '..', '.babelrc' ), 'utf8' )
 );
 babelDefaultConfig.babelrc = false;
-const presetEnvConfig = babelDefaultConfig.presets[ 0 ][ 1 ];
+const presetEnvConfig = babelDefaultConfig.presets[0][1];


Debatable, but overall :(

# Conflicts: # .travis.yml # package.json # scripts/build.js

gziolo · 2018-04-10T05:38:51Z

@ntwb, should we close this one? It is very out of date and we are tackling it a bit differently than initially started.

youknowriad reviewed Jun 28, 2017

View reviewed changes

ntwb and others added 2 commits June 29, 2017 17:34

chore: First pass adding ESLint.

c55e595

Inlcudes a `package.json` and `.gitignore`

chore: Fix linting issues in the build script

69b52ef

ntwb added 2 commits June 30, 2017 19:39

docs: Update CONTRIBUTING.md with npm instructions for ESLint

f2946e3

chore: Use npm test script for Travis CI

4680edf

ntwb mentioned this pull request Jul 3, 2017

Test: Setup unit testing using Jest #9

Merged

ntwb added 2 commits July 3, 2017 21:52

chore: Move ESLint configuration from package.json to standalone `.…

7818996

…eslintrc.json` file.

Merge remote-tracking branch 'WordPress/master' into eslint

118c292

ntwb added 4 commits July 5, 2017 00:16

Merge remote-tracking branch 'WordPress/master' into eslint

d35f3a6

chore: Add jest ESLint environment

16f306c

chore: Override and turn off one-var ESLint rule

b9c8186

refactor: Whitespace fixes via ESLints --fix option.

f5fe1da

gziolo reviewed Jul 5, 2017

View reviewed changes

ntwb self-assigned this Jul 7, 2017

chore: Use babel-eslint as the ESLint parser

c18e23b

ntwb added the Build Tooling label Jul 19, 2017

nylen reviewed Jul 20, 2017

View reviewed changes

nylen suggested changes Jul 20, 2017

View reviewed changes

aduth mentioned this pull request Jul 27, 2017

Add a11y and dom-ready packages. #11

Merged

ntwb added 2 commits August 1, 2017 23:12

Merge remote-tracking branch 'WordPress/master' into eslint

ee1921f

# Conflicts: # .travis.yml # package.json # scripts/build.js

Fix merge conflicts

dc4937c

omarreiss mentioned this pull request Aug 15, 2017

Unapply all spacing exceptions. #35

Merged

gziolo mentioned this pull request Sep 28, 2017

Framework: Should we use Prettier? WordPress/gutenberg#2819

Closed

ntwb closed this Apr 10, 2018

ntwb deleted the eslint branch April 10, 2018 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: First pass adding ESLint. #7

chore: First pass adding ESLint. #7

ntwb commented Jun 28, 2017

youknowriad Jun 28, 2017

ntwb commented Jun 28, 2017

youknowriad Jun 28, 2017

ntwb Jun 28, 2017

youknowriad commented Jun 29, 2017

youknowriad commented Jun 29, 2017

ntwb commented Jun 30, 2017

gziolo commented Jul 2, 2017

ntwb commented Jul 2, 2017

gziolo commented Jul 2, 2017

rmccue commented Jul 2, 2017

gziolo commented Jul 2, 2017 •

edited

Loading

ntwb commented Jul 2, 2017

rmccue commented Jul 2, 2017

gziolo commented Jul 3, 2017

youknowriad commented Jul 3, 2017

gziolo Jul 5, 2017 •

edited

Loading

ntwb Jul 5, 2017

nylen Jul 20, 2017

nylen left a comment

nylen Jul 20, 2017

nylen Jul 20, 2017

nylen Jul 20, 2017

nylen Jul 20, 2017

gziolo commented Apr 10, 2018

chore: First pass adding ESLint. #7

chore: First pass adding ESLint. #7

Conversation

ntwb commented Jun 28, 2017

Choose a reason for hiding this comment

ntwb commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Jun 29, 2017

youknowriad commented Jun 29, 2017

ntwb commented Jun 30, 2017

gziolo commented Jul 2, 2017

ntwb commented Jul 2, 2017

gziolo commented Jul 2, 2017

rmccue commented Jul 2, 2017

gziolo commented Jul 2, 2017 • edited Loading

ntwb commented Jul 2, 2017

rmccue commented Jul 2, 2017

gziolo commented Jul 3, 2017

youknowriad commented Jul 3, 2017

gziolo Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nylen left a 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

gziolo commented Apr 10, 2018

gziolo commented Jul 2, 2017 •

edited

Loading

gziolo Jul 5, 2017 •

edited

Loading