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

AMP Stories: Use Gutenberg ESLint config #1894

Merged
merged 4 commits into from
Feb 21, 2019

Conversation

swissspidy
Copy link
Collaborator

@swissspidy swissspidy commented Feb 21, 2019

While working on #1885 and #1893 I noticed some odd ESLint rules that I thought could be streamlined so they're more in line with PHP and with core.

When looking into it, I found #1019 with relevant commits 277da63 and 34024cc.

I noticed that eslint-config-wordpress is being used as the foundation for the ESLint config, which hasn't seen any new notable updates since 2017. However, a lot has changed since then.

The WordPress JS coding standards have evolved over time, mainly due to Gutenberg development. Gutenberg has way more rules for JS development in WordPress. Some of them which were manually ported over into this repository.

Nowadays, Gutenberg uses and publishes @wordpress/eslint-plugin as a central way to get an up-to-date ESLint config.

For this PR I adapted the config and then tried to fix all linting errors. The diff is quite long, mainly due to the following two reasons:

  • Trailing commas for objects and arrays (like in PHP)
  • Variables that don't change are now const, and not just let or even var.

I think this could be a good starting point to improve the code base in the long term, separate components, and make them testable.

For example, I am thinking about adopting @wordpress/scripts for testing and configuration. Thanks to WordPress/gutenberg#13814 we can simplify #1886 even further.

The former hasn’t been updated in two years. The replacement is much closer to current standards and is also used by Gutenberg and thus WordPress core.
@googlebot googlebot added the cla: yes Signed the Google CLA label Feb 21, 2019
@swissspidy swissspidy marked this pull request as ready for review February 21, 2019 13:46
@westonruter westonruter merged commit e5bc598 into amp-stories-redux Feb 21, 2019
@westonruter westonruter deleted the use-wordpress-eslint-plugin branch February 21, 2019 14:23
@westonruter westonruter added this to the v1.2 milestone May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants