eslint-plugin-json

eslint-plugin-json copied to clipboard

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).

Hi! Thank you for the PR.

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.

IMHO, I prefer this usage design more. ... 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 /flat could be removed, hence causing breakage then.

If /flat remained, then users would have to deal with a slightly different configuration style to all the other ESLint plugins.

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.

@azeemba

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'd like to recommend to take a look over these.

• https://eslint.org/blog/2022/08/new-config-system-part-1
• https://eslint.org/blog/2022/08/new-config-system-part-2
• https://eslint.org/docs/latest/use/configure/configuration-files-new

@Standard8

ESLint is already deprecating the old configuration with v9

I wanted to suspend the breaking change until the release of v9. That's why I suggested /flat.

the implication would be that the /flat could be removed, hence causing breakage then.

/flat should not be removed suddenly. It should just be deprecated in favor of the default entry point. Once deprecated, it should fall back to the default entry point. Deletion should happen later when enough time elapsed so the majority of consumers do not use it. Theoretically breaking, but practically non-breaking.

do the breaking change now

But anyway, if you surely want to create the breaking change right now, I think that's tolerable.

I made changes to the test setup on the main branch so rebased here and force pushed.

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 config.plugins options that v9 allows. Hence I've filed an issue on that as well.

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: https://github.com/not-an-aardvark/eslint-plugin-self/pull/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 ({"allowComments": true}): don't we need a schema entry in the meta property (line 107)?

I tried changing my hack from the issue #80 to enable comments by using the following rule:

    rules: {
      'pluginJson/*': ['error', { allowComments: true }],
    },

and got

Oops! Something went wrong! :(

ESLint: 9.1.1

Error: Key "rules": Key "pluginJson/*":
        Value [{"allowComments":true}] should NOT have more than 0 items.

As the rules can be configured with an option ({"allowComments": true}): don't we need a schema entry in the meta property (line 107)?

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 schema property would make it necessary to add a meta tag too. But all this is already done in this PR (#82).

Besides this we do not have a problem when using eslint before version 9.0. The eslint documentation says:

Note: Prior to ESLint v9.0.0, rules without a schema are passed their options directly from the config without any validation. In ESLint v9.0.0 and later, rules without schemas will throw errors when options are passed.

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:

meta: {
  :
  :
  schema: [
    "anyOf": [
      {
        enum: ["allowComments"]
      },
      {
        type: "object",
        properties: {
          allowComments: { type: "boolean" }
        },
        additionalProperties: false
      }
    ]
  ]
}

So IMHO it would be enough to add the schema to this PR as this PR is for eslint v9 and above.

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.

So IMHO it would be enough to add the schema to this PR as this PR is for eslint v9 and above.

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.

@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:

• Vendor eslint-plugin-self into vendor/eslint-plugin-self. Given the PR there hasn't gone anywhere, I think it is reasonable to fork it to unblock ourselves, especially as it is small. Note that we'll only need eslint-plugin-self for the testing of the old config format with v7/v8. The flat config formats don't need it.
• Renames the existing integration tests for eslint-v7 and eslint-v8 to eslint-v7-legacy and eslint-v8-legacy. This also occurs for the test files.
• Adds a new eslint-v8 which tests the flat configuration.
• Adds the actual flat configuration support.

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! I wanted to discuss a little bit on the logistics on the next steps. This is going to be a breaking change, is that correct? I am checking to make sure that we should publish it as a new major version. Do you have a preference on whether the release should include support for eslint v9 or do you prefer them in separate releases? Is there anything else you want to be included in this PR or the next release?

@Standard8 thanks for getting the flat config working! I wanted to discuss a little bit on the logistics on the next steps. This is going to be a breaking change, is that correct?

Currently it is. The other option for the configurations would be as mentioned earlier to keep the existing configurations named the same and use flat/recommended (or something like that, for the new configuration.

I think they probably both end up about the same. The downside with including flat in the name is that you either keep it in there forever or remove it at some stage (which would still be a breaking change). The downside with swapping to use legacy is that is also a breaking change.

I am checking to make sure that we should publish it as a new major version. Do you have a preference on whether the release should include support for eslint v9 or do you prefer them in separate releases? Is there anything else you want to be included in this PR or the next release?

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.

Am I understand correctly that the current version of eslint-plugin-json fails when running with eslint v9? If so, thanks for flagging that!

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!