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

Update: Implement FlatConfigArray (refs #13481) #14321

Merged
merged 35 commits into from Jun 26, 2021
Merged

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Apr 13, 2021

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:

Implements FlatConfigArray (refs #13481)

What changes did you make? (Give an overview)

I added FlatConfigArray class and associated tests to make sure everything is working okay. FlatConfigArray is not referenced anywhere while running ESLint currently, as implementing FlatConfigArray is meant to be a standalone task. In the next task, I'll work on using it when eslint.config.js is found.

Merging this does not result in any end-user-facing changes. It will result in FlatConfigArray tests being run with npm test

Is there anything you'd like reviewers to focus on?

  1. Are there any cases I missed?
  2. Is the code easy enough to follow? (The old config system was so complicated that the code was hard to follow, I want this version to be easier for people to look at and figure out what's going on.)

@nzakas nzakas added core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion feature This change adds a new feature to ESLint labels Apr 13, 2021
@eslint-github-bot
Copy link

Hi @nzakas!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag must be one of the following:

    The Tag is one of the following:

    • Fix - for a bug fix.
    • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
    • New - implements a new feature.
    • Breaking - for a backwards-incompatible enhancement or feature.
    • Docs - changes to documentation only.
    • Build - changes to build process only.
    • Upgrade - for a dependency upgrade.
    • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

    You can use the labels of the issue you are working on to determine the best tag.

  • There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

@nzakas nzakas changed the title Flat config array Update: Implement FlatConfigArray (refs #13481) Apr 13, 2021
@eslint-github-bot
Copy link

Hi @nzakas!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag must be one of the following:

    The Tag is one of the following:

    • Fix - for a bug fix.
    • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
    • New - implements a new feature.
    • Breaking - for a backwards-incompatible enhancement or feature.
    • Docs - changes to documentation only.
    • Build - changes to build process only.
    • Upgrade - for a dependency upgrade.
    • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

    You can use the labels of the issue you are working on to determine the best tag.

  • There should be a space following the initial tag and colon, for example 'New: Message'.

  • The first letter of the tag should be in uppercase

Read more about contributing to ESLint here

@nzakas nzakas changed the title Update: Implement FlatConfigArray (refs #13481) Update: Implement FlatConfigArray (refs #13481) Apr 13, 2021
@nzakas nzakas requested a review from a team April 13, 2021 18:44
Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

I am pleasantly surprised by how much less is going on here than in the existing config system. Outsourcing all of the file path matching to @humanwhocodes/config-array nicely separates the "what needs to be merged" logic from the "what are the rules for merging each field" logic. It's great to see the vision of a simpler config coming together 🎉

tests/lib/config/flat-config-array.js Show resolved Hide resolved
tests/lib/config/flat-config-array.js Show resolved Hide resolved
tests/lib/config/flat-config-array.js Show resolved Hide resolved
tests/lib/config/flat-config-array.js Show resolved Hide resolved
tests/lib/config/flat-config-array.js Show resolved Hide resolved
lib/config/flat-config-schema.js Outdated Show resolved Hide resolved
lib/config/flat-config-schema.js Show resolved Hide resolved
lib/config/default-config.js Show resolved Hide resolved
lib/config/flat-config-schema.js Show resolved Hide resolved
@nzakas
Copy link
Member Author

nzakas commented Apr 19, 2021

I believe everything has been addressed, but let me know if I missed anything.

Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

I just did a thorough pass through the schema to make sure everything matches up. I'm excited that we'll be able to alert users to invalid configurations as close to the source as possible with some of the checks you've added. There were a few particularly helpful ones that aren't covered by a test, so I pointed those out in case you'd like to add those. LGTM either way.

lib/config/flat-config-schema.js Show resolved Hide resolved
lib/config/flat-config-schema.js Show resolved Hide resolved
lib/config/flat-config-schema.js Show resolved Hide resolved
lib/config/flat-config-array.js Show resolved Hide resolved
@nzakas
Copy link
Member Author

nzakas commented Apr 23, 2021

Nice! I’ll plan on adding tests for those.

Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

Nice! Sorry about the false positive on a couple of those.

rules: defaultRulesConfig
}
},
ignores: ["node_modules/**"],
Copy link
Member

Choose a reason for hiding this comment

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

Is this now a glob pattern, or a .gitignore-style pattern as it used to be with ignorePatterns?

If it's a glob, should it be **/node_modules/*, as an equivalent to the actual default ignore pattern /**/node_modules/*?

Also, ignoring files in the RFC specifies .git directories. Should we add a pattern for them here?

Copy link
Member Author

Choose a reason for hiding this comment

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

These are all glob patterns now. I’ll update them to match the current defaults.

Copy link
Member

Choose a reason for hiding this comment

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

Does it also mean that we'll interpret the content of .eslintignore file differently, depending on the format of the found eslint configuration file:

  • as a list of .gitignore patterns (actual behavior) if the config is .eslintrc.*.
  • as a list of glob patterns if the config is eslint.config.js

Copy link
Member Author

Choose a reason for hiding this comment

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

No. There is no change to how .eslintignore will be interpreted. It will be read and compiled into a function that is then included in a flat config ignores key. (Can be either a glob pattern or a function.)

lib/config/flat-config-array.js Show resolved Hide resolved
lib/config/flat-config-schema.js Outdated Show resolved Hide resolved
lib/config/flat-config-schema.js Outdated Show resolved Hide resolved
@nzakas
Copy link
Member Author

nzakas commented Apr 28, 2021

Comments addressed.

@mdjermanovic
Copy link
Member

@nzakas regarding #14321 (comment), isn't it the opposite (writable - correct, writeable - misspelling)?

Our documentation uses "writable", and says that "writeable" can be used for historical reasons.

lib/config/default-config.js Outdated Show resolved Hide resolved
lib/config/flat-config-schema.js Outdated Show resolved Hide resolved
lib/config/flat-config-schema.js Show resolved Hide resolved
lib/config/rule-validator.js Outdated Show resolved Hide resolved
lib/config/rule-validator.js Outdated Show resolved Hide resolved
lib/config/flat-config-schema.js Outdated Show resolved Hide resolved
lib/config/flat-config-schema.js Outdated Show resolved Hide resolved
lib/config/rule-validator.js Outdated Show resolved Hide resolved
lib/config/flat-config-schema.js Outdated Show resolved Hide resolved
lib/config/default-config.js Outdated Show resolved Hide resolved
lib/config/flat-config-array.js Show resolved Hide resolved
lib/config/rule-validator.js Show resolved Hide resolved
@nzakas
Copy link
Member Author

nzakas commented May 3, 2021

I think I've fixed everything. @mdjermanovic thanks for the amazing level of detail in your comments.

lib/config/flat-config-schema.js Outdated Show resolved Hide resolved
lib/config/flat-config-schema.js Show resolved Hide resolved
@nzakas
Copy link
Member Author

nzakas commented May 7, 2021

Should be set now (hopefully).

@stephenwade stephenwade mentioned this pull request May 11, 2021
1 task
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM!

I'm not sure if the feature of unignoring files that are ignored in the base config (or a shared config) is supported, but we can discuss that when we start integrating.

@mdjermanovic mdjermanovic merged commit b08170b into master Jun 26, 2021
@mdjermanovic mdjermanovic deleted the flat-config-array branch June 26, 2021 11:33
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Dec 24, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants