-
-
Notifications
You must be signed in to change notification settings - Fork 4
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.
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
fix: improve type compatibility with eslint #228
Conversation
This change adjusts the `RuleFunction` generic type to default to `any` for the Node, instead of `never`.
🦋 Changeset detectedLatest commit: d8fb504 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
||
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> = ( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😂
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
This change adjusts the
RuleFunction
generic type to default toany
for the Node, instead ofnever
.Fixes #225