Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Packages: Extract Eslint config package #7965

Merged
merged 5 commits into from
Nov 30, 2018
Merged

Packages: Extract Eslint config package #7965

merged 5 commits into from
Nov 30, 2018

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Jul 14, 2018

Description

This PR extract Eslint config to its own package to make it possible to publish to npm. This also helps us to get rid of a temporary hack (introduced in #5502) of having eslint folder in the root directory of Gutenberg.

Related WordPress JavaScript core chat where we discussed coding standards which should be updated (https://make.wordpress.org/core/2018/7/25/javascript-chat-summary-july-24th/):

  • For multiline comments, we will stick to the current core standards. Aside from some personal preferences, no one raised a strong need to change them.
  • We’ll remove all spacing exceptions from the JS standards on spacing.
  • Yoda conditions will no longer be enforced for JS.
  • We won’t allow any exceptions for strict equality. The current exceptions in the core JS standards will be removed.
  • For multiline conditions we’ll enforce three rules (proposed by @youknowriad):
    • If the condition is short enough to fit in a line, just use a single line
    • If not, one condition per line separated from the opening/closing parenthesis
    • the operator position at the end
  • For multiline conditions we’ll remove the indentation clause from the standards.
  • The Gutenberg naming conventions will be added to the standards. See:
  • CSS naming conventions also need to be normalized with core. This should be addressed separately.
  • We’ll start enforcing trailing comma’s.
  • With regard to variable declarations at the top of each function: var declarations should always be at the top, let and const shouldn’t. This is because var is hoisted, the others aren’t.
  • ES5 (non-transpiled) code should still be able to meet standards. We decided to have two rulesets, one baseset and one set specifically for ESnext (transpiled) code. It should be possible to disable the ESnext ruleset for legacy code. This will also be very useful for plugin and theme authors.
  • The ESnext specific standards still need some more exploration. These will be explored in a trac ticket or Github issue / PR. There is already a related PR here: Packages: Extract Eslint config package #7965
  • We agreed the current JS in core should be updated to adhere as much as possible to the WordPress JS coding standards. This is also in line with the most recent practices regarding coding standards. We decided to at least fix what can be autofixed.

How has this been tested?

  • npm run lint still passes.

Types of changes

Refactoring - no production code has changed.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo gziolo added [Type] Code Quality Issues or PRs that relate to code quality npm Packages Related to npm packages labels Jul 14, 2018
@gziolo gziolo self-assigned this Jul 14, 2018
@gziolo gziolo requested review from youknowriad, ntwb and aduth July 14, 2018 16:12
@gziolo gziolo added this to the 3.3 milestone Jul 14, 2018
ntwb
ntwb previously requested changes Jul 15, 2018
Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

This package is not the "default" ESLint config for WordPress, this package currently describes and uses the current Gutenberg coding standards, these coding standards do not reflect the current WordPress JavaScript coding standards and should not seek to represent that it is.

Side note: There are more instances of eslint-config-default that need to be changed to eslint-config-gutenberg that I did not add code comments that should also be updated.

.eslintrc.js Outdated Show resolved Hide resolved
docs/manifest.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
packages/eslint-config-default/README.md Outdated Show resolved Hide resolved
packages/eslint-config-default/README.md Outdated Show resolved Hide resolved
packages/eslint-config-default/package.json Outdated Show resolved Hide resolved
packages/eslint-config-default/package.json Outdated Show resolved Hide resolved
packages/eslint-config-default/package.json Outdated Show resolved Hide resolved
@aduth
Copy link
Member

aduth commented Jul 17, 2018

From today's Core JS meeting, it was decided to hold off on progress here until divergences between the standards of Gutenberg and WordPress can be enumerated and reconciled.

@gziolo gziolo added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Jul 17, 2018
@gziolo gziolo removed this from the 3.3 milestone Jul 17, 2018
@gziolo
Copy link
Member Author

gziolo commented Jul 23, 2018

We should also address the following comment from @aduth #8132 (comment) as part of this PR:

Note for future reference in extracting rules: We should consider having separate rulesets for ES5 vs. ES2015+ .

Looks like this is already built-in to eslint-plugin-wordpress:

https://github.com/WordPress-Coding-Standards/eslint-plugin-wordpress#available-rulesets

(Confused why there's both a config and a plugin which includes rulesets)

@gziolo
Copy link
Member Author

gziolo commented Nov 27, 2018

I rebased this PR with the latest master changes. In addition I introduced the following changes:

  • renames @wordpress/eslint-config-default to @wordpress/eslint-config
  • inlined all rules for eslint-plugin-wordpress to avoid using git link

@gziolo
Copy link
Member Author

gziolo commented Nov 27, 2018

Next steps we agreed on today during WordPress Core JavaScript weekly chat (https://make.wordpress.org/core/2018/11/27/javascript-chat-summary-november-27-2018/):

  • publish the new @wordpress/eslint-config package.
  • shortly after, or around the same time, publish a make/core post outlining proposed changes to the WordPress javascript standards and references the new published package as the codified representation of them.
  • the post invites feedback/comments on the proposed changes and responses will help tailor how the package is iterated on and also what gets updated in the official javascript standards documentation for WordPress after a week of comments.

@gziolo gziolo removed the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Nov 27, 2018
@ntwb ntwb dismissed their stale review November 27, 2018 21:02

Out of date, out of touch ;)

@gziolo gziolo force-pushed the add/eslint-package branch from 51f203b to d5256b4 Compare November 28, 2018 15:24
.eslintrc.js Outdated Show resolved Hide resolved
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Do we already follow all the discussed guidelines? I'm surprised there's no change in or formatting.

@gziolo
Copy link
Member Author

gziolo commented Nov 29, 2018

Do we already follow all the discussed guidelines? I'm surprised there's no change in or formatting.

It should be closely examined again, but as far as I'm aware all those changes were proposed to make coding styles used in Gutenberg to become a new standard.

@gziolo gziolo force-pushed the add/eslint-package branch from d5256b4 to cfe0005 Compare November 29, 2018 10:45
@gziolo gziolo force-pushed the add/eslint-package branch from cfe0005 to b1cfdc4 Compare November 29, 2018 12:06
'./eslint/config.js',
'plugin:jest/recommended'
],
env: {
Copy link
Member Author

@gziolo gziolo Nov 29, 2018

Choose a reason for hiding this comment

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

It turned out plugin:jest/recommended includes the related env and plugins config :)

https://github.com/jest-community/eslint-plugin-jest/blob/master/index.js#L35-L52

@gziolo gziolo added this to the 4.7 milestone Nov 29, 2018
@gziolo
Copy link
Member Author

gziolo commented Nov 29, 2018

/cc those who discussed at the last WordPress core JavaScript meeting: @aduth @afercia @herregroen @nerrad @omarreiss @youknowriad

Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

A couple of inline suggested changes for ESLint spelling, otherwise 👍

Fix ESLint typo
Kudos to @ntwb for catching

Co-Authored-By: gziolo <grzegorz@gziolo.pl>
@gziolo gziolo merged commit d4d3e0c into master Nov 30, 2018
@gziolo gziolo deleted the add/eslint-package branch November 30, 2018 10:31
@gziolo
Copy link
Member Author

gziolo commented Nov 30, 2018

It might take some time until we publish the alpha version of this package. It probably all depends when WordPress RC is released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Packages Related to npm packages [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants