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

Proposal: merge into eslint-plugin-jsonc #85

Open
JounQin opened this issue Jan 19, 2024 · 9 comments
Open

Proposal: merge into eslint-plugin-jsonc #85

JounQin opened this issue Jan 19, 2024 · 9 comments

Comments

@JounQin
Copy link

JounQin commented Jan 19, 2024

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
Copy link
Owner

azeemba commented Jan 20, 2024

What does "correct" mean in this context?

@JounQin
Copy link
Author

JounQin commented Jan 20, 2024

A passer parses json content into estree compatible AST without preprocess hacking like

return ['']; // sorry nothing ;)

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

@azeemba
Copy link
Owner

azeemba commented Jan 21, 2024

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:

  • what user experiences become feasible because the AST is provided to eslint? Are users already seeing this benefit or is there more work needed for that benefit?
  • what risks are introduced? Does providing the AST increase interactions with other plugins that can conflict? Have the owners of jsonc plugin seen issues like this?

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
Copy link
Author

JounQin commented Jan 21, 2024

@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
Copy link

ota-meshi commented Jan 21, 2024

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
Copy link
Owner

azeemba commented Jan 21, 2024

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
Copy link

ota-meshi commented Jan 22, 2024

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
Copy link
Author

JounQin commented Jan 22, 2024

@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
Copy link
Author

JounQin commented Jan 22, 2024

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

geissonator pushed a commit to openbmc/openbmc-build-scripts that referenced this issue May 1, 2024
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>
geissonator pushed a commit to openbmc/openbmc-build-scripts that referenced this issue May 1, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants