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

feat: Better error message for flat config plugins #17399

Merged
merged 5 commits into from Jul 25, 2023

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Jul 21, 2023

Prerequisites checklist

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

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

What changes did you make? (Give an overview)

Made it so that if the plugins key is defined as an array, then we throw a specific validation message that lets people know it's an eslintrc style config that needs to be converted. I also added a section on the config migration guide to include details on using FlatCompat.

Refs #17370

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

Does the wording make sense?

@nzakas nzakas requested a review from a team as a code owner July 21, 2023 18:00
@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Jul 21, 2023
@netlify
Copy link

netlify bot commented Jul 21, 2023

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 999ae7b
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/64c01b13460be10008ec83fd
😎 Deploy Preview https://deploy-preview-17399--docs-eslint.netlify.app/use/configure/migration-guide
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Comment on lines 6 to 14
A config object has a "plugins" key defined as an array of strings.

Flat config requires "plugins" to be an object in this form:

{
plugins: {
${plugins[0] || "namespace"}: pluginObject
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Flat config schema throws this error on any array (it doesn't check whether it's an array of strings), so if the user specifies plugins like this:

module.exports = [{
    plugins: [somePluginObject]
}];

The printed message will look like this:

Oops! Something went wrong! :(

ESLint: 8.45.0


A config object has a "plugins" key defined as an array of strings.

Flat config requires "plugins" to be an object in this form:

    {
        plugins: {
            [object Object]: pluginObject
        }
    }

Please see the following page for information on how to convert your config object into the correct format:
https://eslint.org/docs/latest/use/configure/migration-guide#importing-plugins-and-custom-parsers

If you're using a shareable config that you cannot rewrite in flat config format, then use the compatibility utility:
https://eslint.org/docs/latest/use/configure/migration-guide#using-eslintrc-configs-in-flat-config

Perhaps we should have two templates?

  • plugins is an array of strings (or empty array?) - possible eslintrc config, use this template
  • plugins is an array but not array of strings - use another template.

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 think for now I'd like to keep things simple and just assume that when people are using an array then they are most likely using an eslintrc-style config. We can always expand later if we find that's not the case.

I've updated to check if the first item in the array is a string to make sure that the message makes sense.

@mdjermanovic mdjermanovic 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 labels Jul 22, 2023
nzakas and others added 2 commits July 24, 2023 10:50
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

LGTM.

@aladdin-add
Copy link
Member

There is a conflict that needs to be resolved.

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 when merge conflicts are resolved.

Copy link
Member

@fasttime fasttime left a comment

Choose a reason for hiding this comment

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

Just a typo, and a merge conflict that needs to be fixed. Otherwise LGTM.

docs/src/use/configure/migration-guide.md Outdated Show resolved Hide resolved
@nzakas
Copy link
Member Author

nzakas commented Jul 25, 2023

Should be all set. Please double-check that the merge makes sense.

@mdjermanovic mdjermanovic merged commit 8ca8b50 into main Jul 25, 2023
22 checks passed
@mdjermanovic mdjermanovic deleted the better-schema-errors branch July 25, 2023 21:25
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jan 22, 2024
@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 Jan 22, 2024
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

4 participants