eslint / json

JSON language plugin for ESLint
Apache License 2.0
44 stars 4 forks source link

fix: Use updated types from @eslint/core #66

Open nzakas opened 2 days ago

nzakas commented 2 days ago

Prerequisites checklist

What is the purpose of this pull request?

Update the plugin to use the latest types from @eslint/core.

What changes did you make? (Give an overview)

Related Issues

Is there anything you'd like reviewers to focus on?

The types test fails with this, and I don't understand why. The project builds just fine. Maybe @fasttime can take a look?

fasttime commented 2 days ago

It looks like the type tests are failing because the strict option is not compatible with the @eslint/core types:

https://github.com/eslint/json/blob/a6c0bc9db1e265484c275860fdb41fcfd8aefaf2/tests/types/tsconfig.json#L6

We could remove that option for the moment since it's only used in the tests. In the long term I think it would be good to make sure that the @eslint/core types are strict compliant, so they can be used by TS projects with that option enabled. As I said in a previous comment, I don't think it's necessary to enable strict mode for the whole rewrite repo.

nzakas commented 2 days ago

It looks like the type tests are failing because the strict option is not compatible with the @eslint/core types

Can you explain what the compatibility issue is? And why does running tsc on the project build fine but the type tests fails?

fasttime commented 2 days ago

Can you explain what the compatibility issue is?

The issue is in the definition of RuleVisitor in @eslint/core:

export interface RuleVisitor {
    /**
     * Called for each node in the AST or at specific times during the traversal.
     */
    [key: string]: (...args: any[]) => void;
}

from https://github.com/eslint/rewrite/blob/3591a7805a060cb130d40d61f200431b782431d8/packages/core/src/types.ts#L102-L107

When the TypeScript strict option is set (or strictNullChecks more specifically) it's no longer possible to assign null or undefined to a property of the RuleVisitorInterface, or to extend it with an optional method (where the value can be a function or undefined).

For example:

let ruleVisitor: RuleVisitor = {};
ruleVisitor.foo = undefined; // Error with strictNullChecks, else OK

interface Bar extends RuleVisitor {
    bar?(): void; // Error with strictNullChecks, else OK
}

The JSONRuleVisitor interface extends RuleVisitor but also defines optional methods, that's why it fails to compile with the strict option.

https://github.com/eslint/json/blob/fdde59d8fcfbc219a38d5e64cdbec61f041e1d98/src/types.ts#L58-L84

It's possible that there are more incompatibilities in the core types but in this is the only one that's causing troubles with this PR. I tried manually changing the RuleVisitor interface to accept optional methods and then all type tests passed:

export type RuleVisitor = {
    /**
     * Called for each node in the AST or at specific times during the traversal.
     */
-   [key: string]: (...args: any[]) => void;
+   [key in string]?: (...args: any[]) => void;
};

And why does running tsc on the project build fine but the type tests fails?

Because the strict option is only enabled in tests/types/tsconfig.json (the config for type tests), not in the root tsconfig.json which is used for building.

nzakas commented 1 day ago

Ah! That is very helpful, thank you.

I think we should change RuleVisitor, then, as I've run into other problems caused by this.