facebook / jscodeshift

A JavaScript codemod toolkit.
https://jscodeshift.com
MIT License
9.36k stars 481 forks source link

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

Open 625dennis opened 2 years ago

625dennis commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

class Bar extends Foo {
  override foo();
}

foo() needs curly braces, or else it can't be found by the query. Otherwise the whole thing comes back unmodified. I wouldn't be surprised if the issue is that ast/recast doesn't yet support override, but the jury's still out on that one.

On Tue, Jul 19, 2022 at 8:54 PM Daniel Lo Nigro @.***> wrote:

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

— Reply to this email directly, view it on GitHub https://github.com/facebook/jscodeshift/issues/513#issuecomment-1189687608, or unsubscribe https://github.com/notifications/unsubscribe-auth/APE6QJQI6XUW2Q573UX3JKDVU5E2TANCNFSM54BOIFWQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

Daniel15 commented 2 years ago

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

ElonVolo commented 2 years ago

There's a few other typos. This works for me to reproduce.

import { Transform } from 'jscodeshift';

const transform: Transform = (file, api) => {
const j = api.jscodeshift;
const root = j.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;

On Tue, Jul 19, 2022 at 9:27 PM Daniel Lo Nigro @.***> wrote:

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

— Reply to this email directly, view it on GitHub https://github.com/facebook/jscodeshift/issues/513#issuecomment-1189704386, or unsubscribe https://github.com/notifications/unsubscribe-auth/APE6QJRWRCISKUJTWWZDCS3VU5IW5ANCNFSM54BOIFWQ . You are receiving this because you were mentioned.Message ID: @.***>

ElonVolo commented 2 years ago

This reproduces it for me.

import { Transform } from 'jscodeshift';

const transform: Transform = (file, api) => {
    const j = api.jscodeshift;
    const root = j.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;

On Tue, Jul 19, 2022 at 9:27 PM Daniel Lo Nigro @.***> wrote:

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

— Reply to this email directly, view it on GitHub https://github.com/facebook/jscodeshift/issues/513#issuecomment-1189704386, or unsubscribe https://github.com/notifications/unsubscribe-auth/APE6QJRWRCISKUJTWWZDCS3VU5IW5ANCNFSM54BOIFWQ . You are receiving this because you were mentioned.Message ID: @.***>

625dennis commented 2 years ago

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 commented 2 years ago

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

ElonVolo commented 2 years ago

class Bar extends Foo {
  override foo();
}

Needs curly braces, or else it can't be found by the query.

On Tue, Jul 19, 2022 at 8:54 PM Daniel Lo Nigro @.***> wrote:

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

— Reply to this email directly, view it on GitHub https://github.com/facebook/jscodeshift/issues/513#issuecomment-1189687608, or unsubscribe https://github.com/notifications/unsubscribe-auth/APE6QJQI6XUW2Q573UX3JKDVU5E2TANCNFSM54BOIFWQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>