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

Scripts: Add missing root flag to the default Eslint config #13483

Merged
merged 2 commits into from
Jan 24, 2019

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Jan 24, 2019

Description

Extracted from #13394 based on a suggestion from @aduth:
https://github.com/WordPress/gutenberg/pull/13394/files#r249797548

This PR adds missing root flag in the default ESlint config which works as explained in the docs (https://eslint.org/docs/user-guide/configuring):

By default, ESLint will look for configuration files in all parent folders up to the root directory. This can be useful if you want all of your projects to follow a certain convention, but can sometimes lead to unexpected results. To limit ESLint to a specific project, place "root": true inside the eslintConfig field of the package.json file or in the .eslintrc.* file at your project’s root level. ESLint will stop looking in parent folders once it finds a configuration with "root": true.

Testing

Not sure how to test with the recent configs added which depend on the .eslintrc.js in the root folder. Otherwise I would remove it and check whether npm run lint-js works.

@gziolo gziolo added [Type] Bug An existing feature does not function as intended [Type] Build Tooling Issues or PRs related to build tooling labels Jan 24, 2019
@gziolo gziolo self-assigned this Jan 24, 2019
@gziolo gziolo force-pushed the update/scripts-eslint-config branch from bb3e158 to e23f449 Compare January 24, 2019 14:32
@gziolo gziolo added this to the 5.0 (Gutenberg) milestone Jan 24, 2019
@gziolo gziolo requested review from a team and aduth January 24, 2019 14:51
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Trying to test this, I still see that ESLint will inherit rules from parent directories.

I think we need to use--either instead of or in addition-- the --no-eslintrc-js parameter when falling back to the default-provided configuration.

https://eslint.org/docs/user-guide/command-line-interface#--no-eslintrc

Here:

[ '--config', fromConfigRoot( '.eslintrc.js' ) ] :

To:

[ '--no-eslintrc', '--config', fromConfigRoot( '.eslintrc.js' ) ] :

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Added the proposed change. Feel free to merge if it makes sense to you.

@gziolo
Copy link
Member Author

gziolo commented Jan 24, 2019

I see this line in ESlint docs and it suddenly makes everything clear:

If .eslintrc.* and/or package.json files are also used for configuration (i.e., -no-eslintrc was not specified), the configurations will be merged. Options from this configuration file have precedence over the options from .eslintrc.* and package.json files.

Thanks for catching it. I didn't think about configs located outside of the project at all. Never stop learning :)

@gziolo
Copy link
Member Author

gziolo commented Jan 24, 2019

Kudos @aduth for changes applied 🥇

I will merge as soon as Travis will turn green.

@gziolo gziolo merged commit 9d7846d into master Jan 24, 2019
@gziolo gziolo deleted the update/scripts-eslint-config branch January 24, 2019 16:53
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Scripts: Add missing root flag to the default Eslint config

* scripts: Instruct ESLint to avoid config discovery in defaulting
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Scripts: Add missing root flag to the default Eslint config

* scripts: Instruct ESLint to avoid config discovery in defaulting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants