Skip to content

fix: improve type compatibility with eslint #228

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 21, 2025
Merged

fix: improve type compatibility with eslint #228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fc1ba25
fix: improve type compatibility with eslint
michaelfaith Sep 4, 2025
d8fb504
add changeset
michaelfaith Sep 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/sour-memes-happen.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"jsonc-eslint-parser": patch
---

improve type compatibility with eslint
3 changes: 2 additions & 1 deletion src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ export interface JSONParserOptions {
jsonSyntax?: JSONSyntax;
}

export type RuleFunction<Node extends AST.JSONNode = never> = (
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- used for compatibility with ESLint types
export type RuleFunction<Node extends AST.JSONNode = any> = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking: would it be good to add a types test to verify this works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The repo is using mocha and doesn't have any solution right now for type testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the past I've hacked it in by adding a .d.ts or .ts file with the expected code, making sure the TS compiler type-checks it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mm, that's a good idea. I'll do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, update. I had a good setup, started, and then realized this repo only has eslint v8 installed, which doesn't even have the latest types to test with 😭. I don't think updating all of the infra in the repo to test this properly is really in scope of the PR.

For what it's worth, this is what I had set up for type checking as part of npm test

import type { ESLint, Rule } from "eslint";
import type { RuleListener } from "../../src/types";

interface RuleContext<Options extends unknown[] = unknown[]>
	extends Rule.RuleContext {
	options: Options;
}

interface RuleModule<RuleOptions extends unknown[] = unknown[]> {
  create(context: RuleContext<RuleOptions>): RuleListener;
  meta: Rule.RuleMetaData;
}

const rule = {
  meta: {
    type: "suggestion",
  },
  create() {
    return {
      JSONProperty(node) {
        console.assert(node.type === "JSONProperty", "should be JSONProperty");
      },
    };
  },
} satisfies RuleModule;

const rules = {
  // eslint-disable-next-line @typescript-eslint/naming-convention -- test
  "test-rule": rule,
};

const plugin = {
  rules,
} satisfies ESLint.Plugin;

// This is just so the plugin variable is used.
console.assert(plugin.rules["test-rule"] === rule, "should be same");

Which, effectively, models a real world plugin use case.

Copy link
Contributor Author

@michaelfaith michaelfaith Sep 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I guess, this could go into a top-level fixtures folder, that sets up a hello-world project with eslint v9 and this file, as a sort of integration / e2e test... But that would be a whole new facet of testing that would need to be maintained, and likely a new CI step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even then, that feels a bit too far for the scope of this change, but open to exploring it, if you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Heh I should probably stop making suggestions, as I don't even maintain this repo 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ota-meshi any preference on this?

Copy link
Owner

Choose a reason for hiding this comment

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

I think tests are necessary.
Without tests, I might accidentally revert this change.
However, creating tests in my current development environment is a pain, so I'll merge this PR. After that, if you can, please help me update my development environment and add tests.

node: Node,
) => void;

Expand Down