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

Build Tooling: Try to extract shared eslint config #5502

Merged
merged 1 commit into from
Mar 12, 2018

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Mar 8, 2018

Description

Follow-up for #5491.

With the ESLint configuration ported to JavaScript, it may be easier to update the E2E-specific lint rules to extend this as a base

Yes, it was very easy :)

Let's agree what should be a shared config and then we can publish to npm as @wordpress/eslint-config-default from the packages repository and consume it here.

Bonus feature: it would be nice to have Jest included in the default setup, but maybe all the rules would be enabled only based on relative glob patterns as described in here: https://eslint.org/docs/user-guide/configuring#configuration-based-on-glob-patterns.

Related to this, we should enforce ESLint rules within our the ESLint file itself. This requires excluding it from .eslintignore via !.eslintrc.js, but this currently produces errors in test/e2e/.eslintrc.js

This should be also possible with the changes proposed in this PR. @aduth feel free to update this PR.

How Has This Been Tested?

npm run lint

Types of changes

Refactoring.

@gziolo gziolo added [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible [Type] Build Tooling Issues or PRs related to build tooling labels Mar 8, 2018
@gziolo gziolo self-assigned this Mar 8, 2018
@gziolo gziolo requested review from ntwb, aduth and aaronjorbin March 8, 2018 13:03
@@ -0,0 +1,165 @@
module.exports = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why it's in the test folder? Should it be a special eslint folder to be extracted to the packages repository later?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a temp thing. I want to have it inside WordPress/packages :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm moving to eslint folder because Jest loads this file as it was a test 😃

module.exports = {
parser: 'babel-eslint',
extends: [
'wordpress',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we copy the WordPress rules here instead of using them as plugin. If I understand correctly this config is meant to be the new WordPress default eslint config.

Copy link
Member Author

Choose a reason for hiding this comment

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

I assume we need to keep both for some time until merge happens. Otherwise, we will have to keep those 2 in sync. I don't have a strong opinion on this one. I think @ntwb will have some opinions here.

@gziolo gziolo force-pushed the try/eslint-shared-config branch from 4db53b4 to 7e48167 Compare March 9, 2018 07:49
@gziolo
Copy link
Member Author

gziolo commented Mar 9, 2018

We had a quick chat with @ntwb and it looks like we can update the existing https://github.com/WordPress-Coding-Standards/eslint-plugin-wordpress/ repository and expose a few new configurations to be consumed as follows:

module.exports = {
    ...
    extends: [
        'plugin:wordpress/recommended',
        'plugin:wordpress/es-next',
        'plugin:wordpress/react',
        'plugin:wordpress/jest',
    ],
    ...
};

Where the 4 configurations would be exposed from eslint-plugin-wordpress and would combine all the existing rules that we have in here.

@gziolo gziolo merged commit 7d8fb68 into master Mar 12, 2018
@gziolo gziolo deleted the try/eslint-shared-config branch March 12, 2018 06:34
@gziolo
Copy link
Member Author

gziolo commented Mar 12, 2018

Let’s extract shared rules to https://github.com/WordPress-Coding-Standards/eslint-plugin-wordpress/ as the next step according to what I commented above.

@aduth
Copy link
Member

aduth commented Mar 13, 2018

To clarify, is the plan for the eslint folder to be temporary?

@gziolo
Copy link
Member Author

gziolo commented Mar 13, 2018

To clarify, is the plan for the eslint folder to be temporary?

Yes, I hope to get it sorted out this month.

@kmgalanakis
Copy link

@gziolo the plan for this is to be used by other plugins too, right?

If so, could you please give a practical example of how this could be done?

Thank you in advance.

@ntwb
Copy link
Member

ntwb commented Mar 15, 2018

@kmgalanakis See #5600 and the linked pull request, it's a WIP and some docs will follow once PRs are merged.

p.s. You also need to npm install eslint-plugin-wordpress in your plugin repo, this isn't in the PR as Gutenberg already has this.

@gziolo
Copy link
Member Author

gziolo commented Mar 16, 2018

@kmgalanakis, it should work similar to unit tests in scripts. First step is to move all Eslint rules to the repository @ntwb linked.

@grappler
Copy link
Member

grappler commented May 2, 2018

@ntwb @gziolo Is there an issue tracking the progress of moving eslint/config.js to eslint-plugin-wordpress? I would like to set up eslint up for a Gutenberg Block plugin I am working on.

@ntwb
Copy link
Member

ntwb commented May 3, 2018

@grappler I'll get something sorted on the weekend for you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants