WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 13 days ago

#31823 accepted task (blessed)

Add ESLint integration

Reported by: westonruter Owned by: netweb
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords:
Focuses: javascript Cc:

Description (last modified by westonruter)

Update: The JSCS project has merged into ESLint. So now an ESLint config is needed to be developed for core as opposed to a JSCS one.


The JSCS project has added a wordpress preset. This could be useful to be included in Core. All it needs is a .jscsrc file located in the root which contains:

{
        "preset": "wordpress",
        "excludeFiles": [
                "**/vendor/**",
                "**.min.js",
                "**/node_modules/**"
        ]
}

Related:
#30153 (PHP_CodeSniffer)
#25187 (JSHnt)
#28543 (Allow stricter JSHint checks for core files)

Attachments (5)

31823.diff (2.5 KB) - added by netweb 6 months ago.
31823.2.diff (6.2 KB) - added by adamsilverstein 5 months ago.
31823.3.diff (6.2 KB) - added by netweb 5 months ago.
31823.4.diff (21.4 KB) - added by netweb 5 months ago.
31823.5.diff (21.4 KB) - added by netweb 5 months ago.

Download all attachments as: .zip

Change History (24)

#1 @westonruter
3 years ago

  • Description modified (diff)

#2 @netweb
3 years ago

Related: #28236

I have some large concerns in adding JSCS in any form to WordPress Core.

I looked into adding JSCS to WordPress Core to fix #28236 back in January. To get familiar with JSCS I looked at jQuery's implementation and upon looking found that jQuery needed some JSCS updates. I created two pull requests for jQuery, one was merged and the second was not (The JSCS maintainers maintain jQuery's implementation of JSCS.)

I walked away from the second and from JSCS in frustration of ideological differences of what I believe should've been merged vs what the JSCS maintainers believe should've been merged.

For reference these are the two jQuery issues:

As I noted in #29792

This is becoming an important issue for us, our tools need to be both functional and maintained and is one part of the reasoning for the change in #31332

I'll add to this by saying that I think any external libraries we use should also match WordPress Core development philosophies and based on my recent JSCS experience via jQuery at this time I don't believe JSCS align with WordPress'.

#3 @westonruter
3 years ago

Just got this update from @hzoo:

If you guys do add it to wordpress core (or even just run it on the codebase) - there will probably be a lot of changes needed to the preset (added and removed). Like #1187.

So you can create an issue/PR about it so we can get it in.

And if there's differences in rules because of jquery changes we could move the original jquery rules into wordpress so it doesn't depend on another preset (if needed).

Maybe it would be interesting to not have to depend on jscs itself for the preset definitions (but that's probably a different thing altogether and maybe already discussed?).

Disclaimer: I'm new to JSCS.

#4 @iseulde
18 months ago

This is now merged with ESLint, see http://jscs.info.

#5 @westonruter
18 months ago

  • Description modified (diff)
  • Summary changed from Add JSCS integration to Add ESLint integration

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


6 months ago

#7 @tfrommen
6 months ago

I just came across this ticket. Not sure how things are going here, but at Inpsyde, we are using ESLint for a longer time already.

Quite a while ago, I created a Modern JavaScript Style Guide that is heavily influenced by the WordPress JavaScript Coding Standards as well as the Airbnb JavaScript Style Guide, which also includes ES6-specific content. Based on that style guide, I then created a complete shareable ESLint config, following WordPress coding standards, and at the same time incorporating modern JavaScript, such as ES6 features. For more background, please refer to an ESLint (for WordPress) introduction post on my personal blog.

I just wanted to leave this here for consideration and/or discussion. The config could at least serve as a starting point, if not really the base of an official WordPress ESLint config.

#8 @westonruter
6 months ago

There also an .eslintrc in wp-dev-lib for reference: https://github.com/xwp/wp-dev-lib/blob/master/.eslintrc

#9 @jorbin
6 months ago

  • Milestone changed from Awaiting Review to 4.9

I think it's time for us to switch to ESLint. It will make the code look more like it's all written by a single person, improving readability, and prepare us for writing more ES2015+ code.

First, we need to solidify an eslint config, then we need to do a project like we did with jshint, and tackle each javascript file one at a time. Then, we can remove jshint and replace all of the grunt calls for jshint with eslint.

@netweb
6 months ago

#10 @netweb
6 months ago

Here's a first pass for a new Grunt task grunt eslint in 31823.diff:

  • It uses version 1.0.0 of the "shared config" which is designed as a "drop in" replacement for WordPress' .jshintrc
  • I designed a roadmap for future releases of eslint-config-wordpress, this would allow users of the shared config to "pin" to a version of the config that would be compatible with their codebase until such time they've updated their codebase and able to move to a newer version of the config
  • Version 1.0.0 - Parity with WordPress' current .jshintrc
  • Version 1.1.0 - Parity with WordPress' current .jshintrc with the extra now deprecated JSHint onevar and trailing options
  • Version 2.0.0 - Parity with the JSCS's WordPress config
  • Version 3.0.0 - Utilize and leverage ESLints rules to add sane defaults and further enhance WordPress' existing coding standards

The Grunt task currently errors on a handful of issues in Gruntfile.js and a Twenty Fourteen JS file, to force the Grunt task to run all the subtasks to see what ESLint errors are shown grunt eslint --force should do the trick.

This ticket was mentioned in Slack in #core by netweb. View the logs.


5 months ago

#12 @adamsilverstein
5 months ago

31823.2.diff fixes the linting errors you noted in gruntfile and twenty-fourteen

@netweb
5 months ago

@netweb
5 months ago

#13 @netweb
5 months ago

31823.4.diff utilises ESLint's --fix option based on the updated eslint-config-wordpress version `2.0.0`

Command: eslint Gruntfile.js --config node_modules/eslint-config-wordpress/index.js --fix

@netweb
5 months ago

#14 @netweb
5 months ago

31823.5.diff is a refresh of the previous two patches with a single change, rather than grunt eslint:jshint for "core" files the task is now named grunt eslint:core

#15 @netweb
5 months ago

  • Owner set to netweb
  • Priority changed from lowest to normal
  • Status changed from new to accepted
  • Type changed from enhancement to task (blessed)

This ticket was mentioned in Slack in #core by melchoyce. View the logs.


6 weeks ago

#17 follow-up: @westonruter
6 weeks ago

Let's wait to fix until 4.9 is shipped. It should be an early task. It should be done the same time as #41057.

For the initial commit, let's just get an .eslintrc into trunk to start.

#18 in reply to: ↑ 17 @netweb
6 weeks ago

  • Milestone changed from 4.9 to Future Release

Replying to westonruter:

Let's wait to fix until 4.9 is shipped. It should be an early task. It should be done the same time as #41057.

For the initial commit, let's just get an .eslintrc into trunk to start.

Punting, the current state of the JS Coding Standards is in flux for another couple of weeks, once that stabilises I'll upload a patch and to make available a new Grunt task for ESLint though keeping it excluded from the build process until a fix is ready to patch all existing JS files.

This ticket was mentioned in Slack in #core-js by adamsilverstein. View the logs.


13 days ago

Note: See TracTickets for help on using tickets.