Skip to content

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

Merged
gziolo merged 2 commits intomasterfrom
update/scripts-eslint-config
Jan 24, 2019
Merged

Scripts: Add missing root flag to the default Eslint config#13483
gziolo merged 2 commits intomasterfrom
update/scripts-eslint-config

Conversation

@gziolo
Copy link
Copy Markdown
Member

@gziolo gziolo commented Jan 24, 2019 •
edited
Loading

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
@gziolo gziolo mentioned this pull request Jan 24, 2019
Closed
5 tasks
aduth
aduth requested changes Jan 24, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@aduth aduth left a comment •
edited
Loading

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:

gutenberg/packages/scripts/scripts/lint-js.js

Line 30 in c153cc4

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

To:

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

aduth
aduth approved these changes Jan 24, 2019
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
This was referenced Feb 4, 2019
Merged
Closed
Merged
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
@gziolo @youknowriad
Scripts: Add missing root flag to the default Eslint config (#13483)
161d3d2 
* 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
@gziolo @youknowriad
Scripts: Add missing root flag to the default Eslint config (#13483)
a052b3f 
* 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

Reviewers

@aduth aduth aduth approved these changes

Assignees

@gziolo gziolo

Labels

[Type] Bug An existing feature does not function as intended [Type] Build Tooling Issues or PRs related to build tooling

Projects

None yet

Milestone

5.0 (Gutenberg)

Development

Successfully merging this pull request may close these issues.

2 participants

@gziolo @aduth