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

[eslint] create NodeListener and extend RuleListener #25399

Merged

Conversation

stas-vilchik
Copy link
Contributor

@stas-vilchik stas-vilchik commented Apr 29, 2018

I've created a new type NodeListener and extend the RuleListener with it. This way I get the code completion when implementing a rule:

screen shot 2018-04-29 at 17 55 20

Right now each handler has the same shape (node: ESTree.Node) => void. Would be great to find a way to infer the specific node type for each property, i.e.

{
  Identifier: (node: ESTree.Identifier) => void,
  IfStatement: (node: ESTree.IfStatement) => void,
  // etc
}

Suggestions are welcome!

@typescript-bot typescript-bot added this to Waiting for Reviewers in Pull Request Status Board Apr 29, 2018
@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Apr 29, 2018
@typescript-bot
Copy link
Contributor

typescript-bot commented Apr 29, 2018

@stas-vilchik Thank you for submitting this PR!

🔔 @pmdartus @j-f1 - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Review in Pull Request Status Board May 4, 2018
@typescript-bot typescript-bot added the Unmerged The author did not merge the PR when it was ready. label May 4, 2018
@typescript-bot
Copy link
Contributor

typescript-bot commented May 4, 2018

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

@typescript-bot typescript-bot moved this from Review to Check and Merge in Pull Request Status Board May 4, 2018
@typescript-bot typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Merge:Express and removed Awaiting reviewer feedback Unmerged The author did not merge the PR when it was ready. labels May 4, 2018
@sheetalkamat sheetalkamat merged commit 4b58913 into DefinitelyTyped:master May 7, 2018
@typescript-bot typescript-bot removed this from Check and Merge in Pull Request Status Board May 7, 2018
@@ -241,7 +241,10 @@ export namespace Rule {
meta?: RuleMetaData;
}

interface RuleListener {
type NodeTypes = ESTree.Node['type'];
type NodeListener = { [T in NodeTypes]?: (node: ESTree.Node) => void };
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to type node to be T?

(I'm a TS newbie, so might not be needed).

const rule: Rule.RuleModule = {
  create() {
    return {
      CallExpression(node: CallExpression) {
        console.log(node.callee);
      },
    };
  },
};

Gives me Type 'Node' is not assignable to type 'CallExpression'

Copy link
Contributor

Choose a reason for hiding this comment

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

const rule: Rule.RuleModule = {
  create() {
    return {
      CallExpression(node: CallExpression) {
        if (node.type !== 'CallExpression') {
          return;
        }
        console.log(node.callee);
      },
    };
  },
};

Seems to fix it, but that's dead code...

Copy link
Contributor

@j-f1 j-f1 Nov 27, 2018

Choose a reason for hiding this comment

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

The solution to this is a two-parter:

  1. Add a new mapping to @types/estree. This maps from the name of the node to its type.
    // yes, this is necessary :(
    type _NodeMap = { [K in Node['type']]: Node }
    
    export interface NodeByType extends _NodeMap {
      Identifier: Identifier
    
      SimpleLiteral: SimpleLiteral
      RegExpLiteral: RegExpLiteral
    
      Program: Program
    
      FunctionDeclaration: FunctionDeclaration
      FunctionExpression: FunctionExpression
      ArrowFunctionExpression: ArrowFunctionExpression
    
      SwitchCase: SwitchCase
      CatchClause: CatchClause
      VariableDeclarator: VariableDeclarator
    
      ExpressionStatement: ExpressionStatement
      BlockStatement: BlockStatement
      EmptyStatement: EmptyStatement
      DebuggerStatement: DebuggerStatement
      WithStatement: WithStatement
      ReturnStatement: ReturnStatement
      LabeledStatement: LabeledStatement
      BreakStatement: BreakStatement
      ContinueStatement: ContinueStatement
      IfStatement: IfStatement
      SwitchStatement: SwitchStatement
      ThrowStatement: ThrowStatement
      TryStatement: TryStatement
      WhileStatement: WhileStatement
      DoWhileStatement: DoWhileStatement
      ForStatement: ForStatement
      ForInStatement: ForInStatement
      ForOfStatement: ForOfStatement
    
      // FunctionDeclaration: FunctionDeclaration
      VariableDeclaration: VariableDeclaration
      ClassDeclaration: ClassDeclaration
    
      ThisExpression: ThisExpression
      ArrayExpression: ArrayExpression
      ObjectExpression: ObjectExpression
      // FunctionExpression: FunctionExpression
      // ArrowFunctionExpression: ArrowFunctionExpression
      YieldExpression: YieldExpression
      // Literal: Literal
      UnaryExpression: UnaryExpression
      UpdateExpression: UpdateExpression
      BinaryExpression: BinaryExpression
      AssignmentExpression: AssignmentExpression
      LogicalExpression: LogicalExpression
      MemberExpression: MemberExpression
      ConditionalExpression: ConditionalExpression
      CallExpression: SimpleCallExpression // <-- Important!
      NewExpression: NewExpression
      SequenceExpression: SequenceExpression
      TemplateLiteral: TemplateLiteral
      TaggedTemplateExpression: TaggedTemplateExpression
      ClassExpression: ClassExpression
      MetaProperty: MetaProperty
      // Identifier: Identifier
      AwaitExpression: AwaitExpression
    
      Property: Property
      AssignmentProperty: AssignmentProperty
      Super: Super
      TemplateElement: TemplateElement
      SpreadElement: SpreadElement
    
      // Identifier: Identifier
      ObjectPattern: ObjectPattern
      ArrayPattern: ArrayPattern
      RestElement: RestElement
      AssignmentPattern: AssignmentPattern
      // MemberExpression: MemberExpression
    
      ClassBody: ClassBody
      // ClassDeclaration: ClassDeclaration
      // ClassExpression: ClassExpression
      MethodDefinition: MethodDefinition
    
      ImportDeclaration: ImportDeclaration
      ExportNamedDeclaration: ExportNamedDeclaration
      ExportDefaultDeclaration: ExportDefaultDeclaration
      ExportAllDeclaration: ExportAllDeclaration
    
      ImportSpecifier: ImportSpecifier
      ImportDefaultSpecifier: ImportDefaultSpecifier
      ImportNamespaceSpecifier: ImportNamespaceSpecifier
      ExportSpecifier: ExportSpecifier
    }
  2. Redefine the listener:

    type NodeListener = {
      [T in ESTree.Node['type']]?: (node: ESTree.NodeByType[T]) => void
    }

I defined the mapping in @types/estree so it gets updated as new nodes are added.

Copy link
Contributor

Choose a reason for hiding this comment

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

@j-f1 could you send a PR for that? 👼

Copy link
Contributor

Choose a reason for hiding this comment

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

@SimenB I got started here, but I couldn’t get the tests to pass :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Owner Approved A listed owner of this package signed off on the pull request. Popular package This PR affects a popular package (as counted by NPM download counts).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants