azeemba / eslint-plugin-json

Lint your JSON files
MIT License
209 stars 29 forks source link

Proposal: merge into `eslint-plugin-jsonc` #85

Open JounQin opened 9 months ago

JounQin commented 9 months ago

In case you are not aware, there is a https://github.com/ota-meshi/eslint-plugin-jsonc built with correct jsonc-eslint-parser, I think we should work together to benefit to the whole community.

cc @ota-meshi

azeemba commented 9 months ago

What does "correct" mean in this context?

JounQin commented 9 months ago

A passer parses json content into estree compatible AST without preprocess hacking like https://github.com/azeemba/eslint-plugin-json/blob/6fc5651b4fb58ecf532c569938c1dc2dda771fe1/src/index.js#L115

See also https://github.com/ota-meshi/jsonc-eslint-parser/blob/master/docs/AST.md

azeemba commented 9 months ago

Cool, thanks for the explanation!

In my opinion, this is not as much about correctness as it is about tradeoffs. The problem is that we don't want eslint to apply JavaScript rules to the JSON. This plugin does it by passing in an empty AST and jsonc plugin does it by passing in an AST where the nodes are named so that they don't conflict with the JS equivalents.

What would help this discussion is if someone can provide the cost/benefit of these options. For example, I would love to know:

I see that the jsonc plugin gets a million monthly downloads so that does give me confidence that it is battle-tested. I just want to fully understand the trade-offs before moving further.

JounQin commented 9 months ago

@azeemba

See also https://github.com/JoshuaKGoldberg/eslint-plugin-package-json which already uses that parser for a long time.

The approach used by current plugin conflicts with eslint-plugin-json, when using eslint-plugin-json separately, no conflicts are expected AFAIK.

cc @ota-meshi

My two cents, eslint-plugin-json is more well known and suitable for the package name, if the two plugins can just be merged into a single project, that's really be awesome.

ota-meshi commented 9 months ago

I know that eslint-plugin-json has been and still a good solution for many users who want to lint JSON for quite some time. However, in my opinion, using AST for linting any language is more consistent with the future direction of the recently added RFC for ESLint: https://github.com/eslint/rfcs/blob/main/designs/2022-languages/README.md

I think the advantage of using AST is that someone can create another plugin that uses that AST. I think that if we remove the code in the processor, other plugins will not be able to know the original JSON contents. If we only goal is to resolve conflicts between eslint-plugin-json and other plugins, it may be enough to just remove the part of the processor that removes JSON code.

azeemba commented 9 months ago

Thanks for the link to the RFC! It is particularly helpful since eslint-plugin-json is explicitly mentioned as a plugin that can benefit from this design.

@ota-meshi If we wanted eslint-plugin-json and eslint-plugin-jsonc to be more closely aligned, what strategy would you recommend?

One possible idea would be for eslint-plugin-json to be the same code as eslint-plugin-jsonc but just with different defaults (so json doesn't support comments by default). I guess with this strategy, I don't know what would happen if a user installed both extensions.

I personally don't mind giving up control of eslint-plugin-json, particularly since @ota-meshi seems to be a very active contributor in this space. I would just like to make sure we have a good plan for how the transition would work

ota-meshi commented 9 months ago

If we wanted eslint-plugin-json and eslint-plugin-jsonc to be more closely aligned, what strategy would you recommend?

I would just like to make sure we have a good plan for how the transition would work

Sorry, I haven't thought about that strategy yet 😅

One possible idea would be for eslint-plugin-json to be the same code as eslint-plugin-jsonc but just with different defaults (so json doesn't support comments by default).

Does one plugin have dependencies on the other and share the code? If so, that strategy sounds good to me 😄 @JounQin do you have any ideas?

One thing I'm concerned about is that a JSON linter might be created in the ESLint core org 🤔 https://github.com/eslint/rfcs/blob/main/designs/2022-languages/README.md#will-the-eslint-team-create-and-maintain-additional-languages

JounQin commented 9 months ago

@ota-meshi

My two cents, we should keep only one brand eslint-plugin-json, and determine which parser/language should be considered based on

  1. hard coding, .json as json, .jsonc as jsonc, .json5 as json5, while listing some exceptions for example tsconfig.*.json and angular.json as jsonc
  2. extract all json related languages from linguist-languages on build without depending the whole linguist-languages package
  3. just mark linguist-languages as dependency

Ideally we should parse in loose mode, but report syntax errors in strict mode.

On the merge strategy, if @azeemba agree to abandon/deprecate the current project, we can check is there any rule missing in current eslint-plugin-jsonc, and we should adopt them. And then we should switch the brands

  1. deprecate and archive current eslint-plugin-json project
  2. invite @azeemba as a new member of current eslint-plugin-jsonc project
  3. rename eslint-plugin-jsonc to eslint-plugin-json, @azeemba invites us to be npm onwers of the package
  4. release a new major version of eslint-plugin-json and deprecated eslint-plugin-jsonc package by aliasing
  5. I don't know if it's possible to rename jsonc-eslint-parser to json-eslint-parser due to npm package name policy, but I'm using eslint-mdx as the parser name, and json-eslint can also be considered like babel-eslint

One thing I'm concerned about is that a JSON linter might be created in the ESLint core org 🤔

That won't affect us because we're making json linting working on eslint <9.

JounQin commented 9 months ago

Besides, I think we should move the final project to be a part of @eslint-community.