-
Notifications
You must be signed in to change notification settings - Fork 29
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
Fixes #80: Support ESLint's flat configuration format #82
Conversation
Hi! IMHO, I prefer this usage design more. import json from 'eslint-plugin-json/flat' // <-- /flat entrypoint
export default [
json.configs.recommended,
{
// and/or custom config
files: ['**/*.json'],
plugins: { json },
processor: 'json/json',
rules: { /* ... */}
},
] This can avoid the breaking change as well. |
ESLint is already deprecating the old configuration with v9, therefore I think it makes more sense to do the breaking change now. Otherwise, when removing the legacy configuration support, the implication would be that the If Additionally creating the breaking change now would help to highlight that this plugin is now supporting flat config fully. |
Thanks a lot for opening this PR! I honestly haven't kept up with eslint so I will first try to familiarize myself with what eslint is changing and then test out this PR. |
I made changes to the test setup on the main branch so rebased here and force pushed. |
I'd like to recommend to take a look over these. |
I wanted to suspend the breaking change until the release of v9.
But anyway, if you surely want to create the breaking change right now, I think that's tolerable. |
Thanks, I took a look and noticed one issue with the tests that I've raised as #84 as that can be fixed separately. I also realised that eslint-plugin-self is broken with v9 as it doesn't handle the different I don't know how long it'll take for them to fix that, though I'm also playing around with an alternative fix which wouldn't use eslint-plugin-self. I think it might work, but I need a bit more time to poke at it. |
Eventually got back around to this... I've figured out a fix for eslint-plugin-self - found here: not-an-aardvark/eslint-plugin-self#9 We'll see if that gets accepted or not. With that fix, I've now got the v7 and v8 tests passing. For the v9 tests, I think I have a way forward, will hopefully get time later this weekend. |
As the rules can be configured with an option ( I tried changing my hack from the issue #80 to enable comments by using the following rule:
and got
|
Could you maybe file a separate PR for that? Fixing that can happen separately to the main configuration issues. |
The question is, if a separate PR makes sense. By now in this plugin we do not have a meta property defined. So o make a PR about the missing Besides this we do not have a problem when using eslint before version 9.0. The eslint documentation says:
So IMHO it would be enough to add the schema to this PR as this PR is for eslint v9 and above. The schema is a property of the meta tag (see documentation) and would look like this:
|
Openbmc repo CI uses `eslint-plugin-json` package[1] for JSON linting, and it appears that its broken for eslint@latest versions that mandated the use of flat config. There is some on-going work on fixing the plugin to add flat configuration support[2], but its still a work in progress. This commits also removed `eslint-plugin-json` package and leverages `eslint-plugin-jsonc` package[3], which is better maintained & also uses the same AST as Javascript for linting.Since the plugin could provide AST & source code text back to the eslint engine, we can now use eslint directives such as `/*eslint-disable <checks>*/` in json files. Also it does seem like there are plans to merge[4] `eslint-plugin-json` into `eslint-plugin-jsonc`. Flat configuration model deprecates the use of `.eslintrc.json` and also `.eslintignore`. Instead eslint now relies on just one java script based configuration file `eslint.config.mjs` for everything. So we had to now add some logic into the configuration file to append the repo specific ignores to the global ignores, which forced us to bring yet another node js package `fs`. Since latest versions of eslint flags a warning if there is a presence of deprecated `.eslintignore` file in repositories,I renamed it from `.eslintignore` to `.eslintIgnore`. [1]: https://www.npmjs.com/package/eslint-plugin-json [2]: azeemba/eslint-plugin-json#82 [3]: https://www.npmjs.com/package/eslint-plugin-jsonc [4]: azeemba/eslint-plugin-json#85 Change-Id: I0fa8c928a7449e08d761022dde1f1da3ee48cf62 Signed-off-by: Manojkiran Eda <manojkiran.eda@gmail.com>
Openbmc repo CI uses `eslint-plugin-json` package[1] for JSON linting, and it appears that its broken for eslint@latest versions that mandated the use of flat config. There is some on-going work on fixing the plugin to add flat configuration support[2], but its still a work in progress. This commits also removed `eslint-plugin-json` package and leverages `eslint-plugin-jsonc` package[3], which is better maintained & also uses the same AST as Javascript for linting.Since the plugin could provide AST & source code text back to the eslint engine, we can now use eslint directives such as `/*eslint-disable <checks>*/` in json files. Also it does seem like there are plans to merge[4] `eslint-plugin-json` into `eslint-plugin-jsonc`. Flat configuration model deprecates the use of `.eslintrc.json` and also `.eslintignore`. Instead eslint now relies on just one java script based configuration file `eslint.config.mjs` for everything. So we had to now add some logic into the configuration file to append the repo specific ignores to the global ignores, which forced us to bring yet another node js package `fs`. Since latest versions of eslint flags a warning if there is a presence of deprecated `.eslintignore` file in repositories,I renamed it from `.eslintignore` to `.eslintIgnore`. [1]: https://www.npmjs.com/package/eslint-plugin-json [2]: azeemba/eslint-plugin-json#82 [3]: https://www.npmjs.com/package/eslint-plugin-jsonc [4]: azeemba/eslint-plugin-json#85 Change-Id: I0fa8c928a7449e08d761022dde1f1da3ee48cf62 Signed-off-by: Manojkiran Eda <manojkiran.eda@gmail.com>
I'm trying to limit this PR to only be about flat-config. Flat config is supported in the latest v8. v9 issues can be handled separately. |
Actually, that's probably not quite true, since I was trying to get the v9 tests to pass as well. However, I think it'd still help if stuff like that was done in a separate PR - that could have been landed by now and seeing as you already have the code, it probably wouldn't have taken long. |
6309608
to
b66b033
Compare
@azeemba I finally found some time and managed to figure out the test issues. I have focussed on making this able to work with flat config which is available in ESLint 8.57.0. I have not looked at the v9 specific parts yet, but I suspect with the additional changes here, they will be a lot simpler to finish off. Here's an outline of what this PR now does:
I'm quite happy to change naming if you want. |
@BePo65 Am I understand correctly that the current version of eslint-plugin-json fails when running with eslint v9? If so, thanks for flagging that! As @Standard8 suggested, I think we can get that landed in a separate PR soon. Let us know if you are interested in working on that fix! @Standard8 thanks for getting the flat config working! |
Currently it is. The other option for the configurations would be as mentioned earlier to keep the existing configurations named the same and use I think they probably both end up about the same. The downside with including
I'm fine with either way. I mainly wanted to keep this PR as small as reasonably possible, and punting on the v9 changes helps that. I haven't looked in detail, but I'm fairly sure any additional changes for the v9 should be relatively simple - especially now the testing parts are figured out. I'd be happy to help out with v9 after landing this, but seeing as GitHub doesn't do dependencies between PRs very well, I thought it was worth landing this bit first. |
I am using this plugin in a small project (an upcoming release of license-report) with eslint v9 and flat config and it runs like a charm as long as I don't lint json files with comments (e.g. files like the VScode "launch.json" file). |
I will merge this PR now but won't push a release immediately. I will try to create a patch to add the schema support based on the snippet provided @BePo65. If I am able to get that working, will aim to push a new major version after that change. Thanks again for the PR @Standard8! |
This work has been released as v4.0.0! |
This adds support for the new ESLint flat configuration as per their migration guide: https://eslint.org/docs/latest/extend/plugin-migration-flat-config
In this I've included backwards compatibility for the legacy configuration format. However, this should require a major version bump as the legacy configuration name has changed (
recommended
->recommended-legacy
as suggested in the ESLint docs).