fix: provide a language-specific definition for RuleModule

[fasttime]

The RuleModule type exported by this package is currently the same as ESLint's Rule.RuleModule, meaning that it contains definitions that are specific for the JavaScript language, and so it isn't useful for typing Markdown-specific rules.

This pull request updates the exported RuleModule type, making it specific for the Markdown language.

fixes: #323

[fasttime]

RuleModule is now an alias for the new MarkdownRuleDefinition type with the default generic parameters. It's still there for compatibility but I think it's now confusing (it's no longer identical to ESLint's RuleModule). So I'm not sure if we should also export MarkdownRuleDefinition as a generic type.

[nzakas]

Yeah, I'd say just use MarkdownRuleDefinition. People can always switch to ESLint's version of RuleModule if they really need it.

[fasttime]

I've added an export for MarkdownRuleDefinition as a generic type in 896a194. Now this can be used to define a custom Markdown rule with RuleOptions, MessageIds and ExtRuleDocs, all optional properties.

[fasttime]

Note: we need to discuss the implementation details for MarkdownRuleDefinition along with JSRuleDefinition in eslint/eslint#19521.

[fasttime]

This is required for RuleContext to be the same type as in ESLint. See https://github.com/eslint/eslint/blob/v9.22.0/lib/types/index.d.ts#L44-L64.

It took me a while to understand that the markdown plugin object would not satisfy ESLint.Plugin because this patch was missing:

markdown/tests/types/types.test.ts

Line 8 in f88ed35

markdown satisfies ESLint.Plugin;

[nzakas]

I'm not sure I understand this. When used with ESLint, won't ESLint's definition for RuleContext already be used?

Note: we don't do this for the JSON plugin.

[fasttime]

I'm not sure I understand this. When used with ESLint, won't ESLint's definition for RuleContext already be used?

It looks like the additional definitions for RuleContext are only applied globally when they are located in an ESM module. For example, if I change the file extension to CommonJS (core-patch.d.ts -> core-patch.d.cts), the type test fails again. I suppose this has to do with the fact that CommonJS files have a root scope that is not the global scope, and so the additions in the ESLint types only affect the RuleContext used in that file.

Note: we don't do this for the JSON plugin.

In the JSON plugin, the type test that checks if ESLint.Plugin is satisfied is currently disabled: https://github.com/eslint/json/blob/json-v0.11.0/tests/types/types.test.ts. I think we could re-enable it if we patch the RuleContext also there.

[nzakas]

I'm not liking the idea or propagating JS-specific changes to all of the language plugins that we're just going to have to remove later.

Is there something we can do in the main ESLint repo to make the definition of Plugin not require RuleContext to have these extra methods?

[fasttime]

I think we could add the deprecated methods directly to Rule.RuleContext and update Rule.RuleModule so that the create method accepts an argument of type Rule.RuleContext (as opposed to RuleContext from core types). I can look into how to get this to work.

[fasttime]

Here is a pull request that should fix the issue in the main repo: eslint/eslint#19531.

[nzakas]

Nice!

[lumirlumir]

Thanks you @fasttime

https://github.com/eslint/eslint/blob/main/lib/types/index.d.ts#L185-L284

I believe the IMarkdownSourceCode interface should follow the same format as the SourceCode class defined in the link above.

details

// `types/index.d.ts` of ESLint

export class SourceCode implements TextSourceCode<{
    LangOptions: Linter.LanguageOptions;
    RootNode: AST.Program;
    SyntaxElementWithLoc: AST.Token | ESTree.Node;
    ConfigNode: ESTree.Comment;
}> {
    text: string;
    ast: AST.Program;
    lines: string[];
    hasBOM: boolean;
    parserServices: SourceCode.ParserServices;
    scopeManager: Scope.ScopeManager;
    visitorKeys: SourceCode.VisitorKeys;

    constructor(text: string, ast: AST.Program);
    constructor(config: SourceCode.Config);

    static splitLines(text: string): string[];

    getLoc(syntaxElement: AST.Token | ESTree.Node): ESTree.SourceLocation;
    getRange(syntaxElement: AST.Token | ESTree.Node): SourceRange;

    getText(node?: ESTree.Node, beforeCount?: number, afterCount?: number): string;

    getLines(): string[];

    getAllComments(): ESTree.Comment[];

    getAncestors(node: ESTree.Node): ESTree.Node[];

    getDeclaredVariables(node: ESTree.Node): Scope.Variable[];

    getJSDocComment(node: ESTree.Node): ESTree.Comment | null;

    getNodeByRangeIndex(index: number): ESTree.Node | null;

    isSpaceBetweenTokens(first: AST.Token, second: AST.Token): boolean;

    getLocFromIndex(index: number): ESTree.Position;

    getIndexFromLoc(location: ESTree.Position): number;

    // Inherited methods from TokenStore
    // ---------------------------------

    getTokenByRangeStart(offset: number, options?: { includeComments: false }): AST.Token | null;
    getTokenByRangeStart(offset: number, options: { includeComments: boolean }): AST.Token | ESTree.Comment | null;

    getFirstToken: SourceCode.UnaryNodeCursorWithSkipOptions;

    getFirstTokens: SourceCode.UnaryNodeCursorWithCountOptions;

    getLastToken: SourceCode.UnaryNodeCursorWithSkipOptions;

    getLastTokens: SourceCode.UnaryNodeCursorWithCountOptions;

    getTokenBefore: SourceCode.UnaryCursorWithSkipOptions;

    getTokensBefore: SourceCode.UnaryCursorWithCountOptions;

    getTokenAfter: SourceCode.UnaryCursorWithSkipOptions;

    getTokensAfter: SourceCode.UnaryCursorWithCountOptions;

    getFirstTokenBetween: SourceCode.BinaryCursorWithSkipOptions;

    getFirstTokensBetween: SourceCode.BinaryCursorWithCountOptions;

    getLastTokenBetween: SourceCode.BinaryCursorWithSkipOptions;

    getLastTokensBetween: SourceCode.BinaryCursorWithCountOptions;

    getTokensBetween: SourceCode.BinaryCursorWithCountOptions;

    getTokens:
        & ((node: ESTree.Node, beforeCount?: number, afterCount?: number) => AST.Token[])
        & SourceCode.UnaryNodeCursorWithCountOptions;

    commentsExistBetween(
        left: ESTree.Node | AST.Token | ESTree.Comment,
        right: ESTree.Node | AST.Token | ESTree.Comment,
    ): boolean;

    getCommentsBefore(nodeOrToken: ESTree.Node | AST.Token): ESTree.Comment[];

    getCommentsAfter(nodeOrToken: ESTree.Node | AST.Token): ESTree.Comment[];

    getCommentsInside(node: ESTree.Node): ESTree.Comment[];

    getScope(node: ESTree.Node): Scope.Scope;

    isSpaceBetween(
        first: ESTree.Node | AST.Token,
        second: ESTree.Node | AST.Token,
    ): boolean;

    markVariableAsUsed(name: string, refNode?: ESTree.Node): boolean;

    traverse(): Iterable<TraversalStep>;
}

Since context.sourceCode in @eslint/markdown lacks some methods that exist in the SourceCode class mentioned above, the detailed implementation of IMarkdownSourceCode should differ from ESLint's original SourceCode class.

For example,

• Methods not implemented:

  • sourceCode.getLocFromIndex()
  • sourceCode.getIndexFromLoc()
  • (There may be more missing methods.)

• Methods implemented:

  • sourceCode.getText()
  • sourceCode.getRange()
  • (There may be additional implemented methods.)

---

In conclusion, we need to identify which methods and properties have been correctly implemented.

And also, I believe the node type should be from mdast, rather than a general object type.

---

My opinion may not be entirely correct, so please let me know if further clarification is needed!

[fasttime]

Actually I was looking at IJSONSourceCode as declared in @eslint/json (see https://github.com/eslint/json/blob/json-v0.11.0/src/types.ts#L89-L118) as a model for IMarkdownSourceCode. ESLint types contain legacy code so they're not always the best example. But you are perfectly right that the type of nodes should be Node from mdast, rather than a generic object. That should be fixed in c015363. Please, have a look.

In conclusion, we need to identify which methods and properties have been correctly implemented.

You can check src/language/markdown-source-code.js for the actual implementation. Note that part of the logic is inherited from classes in another package.

[nzakas]

Thanks for taking a look at this. It was in the back of my mind that we needed types for this plugin.

Left a few comments.

[nzakas]

Yeah, I'd say just use MarkdownRuleDefinition. People can always switch to ESLint's version of RuleModule if they really need it.

[nzakas]

Can we add some of the methods with node names like we do for the JSON plugin?
https://github.com/eslint/json/blob/10882ffe9c39cdd866be51801f9950f4a010cd87/src/types.ts#L58-L84

[fasttime]

Done in 4e326af. I've added listeners for all the node types used in our rules plus Root. Please, check if there is anything else that should be added.

[nzakas]

I'm not sure I understand this. When used with ESLint, won't ESLint's definition for RuleContext already be used?

Note: we don't do this for the JSON plugin.

[nzakas]

@fasttime this is still listed as a draft. Are you ready for review?

[fasttime]

Yes, this is ready for review now. I removed the MarkdownRuleDefinition export for the moment because I think we should discuss in eslint/eslint#19521 how to provide consistent implementations for different languages before exposing that type. I will re-add it in a new PR when we have a decision.

[nzakas]

LGTM. Thanks.