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

jscodeshift removes 'override' keyword from ClassMethod when setting returnType #513

Open
625dennis opened this issue Jul 19, 2022 · 8 comments

Comments

@625dennis
Copy link

625dennis commented Jul 19, 2022

Given an input:

class Bar extends Foo {
  override foo();
}

and a transform:

import { Transform } from 'jscodeshift';

const transform: Transform = (file, api) => {
    const jscodeshift = api.jscodeshift;
    const root = jscodeshift.withParser('ts')(file.source);
    
    root.find(j.ClassMethod).forEach(path => {
        path.value.returnType = j.tsTypeAnnotation(j.tsTypeReference(j.identifier('string')));
    });

    return root.toSource({ parser: 'ts' });
};

export default transform;

This will output the class with typed ClassMethod definition but without override:

class Bar extends Foo {
  foo(): string;
}

I expect override to be carried over. One thing I noticed was the jscodeshift types and ast-types types for methods do not include an override boolean property, but during runtime ClassMethod object will contain override: true property if the method signature includes override keyword.

@Daniel15
Copy link
Member

Daniel15 commented Jul 20, 2022

I can't repro on ASTExplorer when using the TypeScript parser: https://astexplorer.net/#/gist/5874ee54ffac67d5b889eda5b82ee883/cd476bad72ae8c6f5164483f93172bd8699590af

ASTExplorer is using an older version of JSCodeshift though, so I wonder if this is a regression in a newer version?

@ElonVolo
Copy link
Collaborator

ElonVolo commented Jul 20, 2022 via email

@Daniel15
Copy link
Member

Ah, good catch @ElonVolo! Now I can repro. https://astexplorer.net/#/gist/5874ee54ffac67d5b889eda5b82ee883/5bb310ff6c635a7963f2c340975c50612550c3d8

@ElonVolo
Copy link
Collaborator

ElonVolo commented Jul 20, 2022 via email

@ElonVolo
Copy link
Collaborator

ElonVolo commented Jul 20, 2022 via email

@625dennis
Copy link
Author

Looks like j.withParser('ts') will use @babel/parser with typescript option as the parser https://github.com/facebook/jscodeshift/blob/514f8c3e4e2e2801713beff93a04f8f8a771fe96/parser/ts.js . https://astexplorer.net/#/gist/a5f747da23a3539448fb63ffe9f04c43/5be0fb3874deba008cd4251e2756b8adb9289bea
Results in same output, but this AST matches what I get in my debug env.

@625dennis
Copy link
Author

Recast doesn't include an override flag in the printMethod function https://github.com/benjamn/recast/blob/25351f6a84cb7c83e77717346487a788a3465204/lib/printer.ts#L2721

@ElonVolo
Copy link
Collaborator

ElonVolo commented Oct 11, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants