Calling .replaceWithText(getFullText(...)) is inconsistent

[abirmingham]

Describe the bug

Version: 4.0.1

Hello! I'm trying to migrate all export default cases in my codebase to named exports. In order to do so, I'm using the following code:

ts-morph code

Array.from(file.getExportedDeclarations().entries()).forEach(
    ([id, declaration]) => {
        if (id !== 'default') return;
        if (
            !_.isEqual(declaration.map(x => x.getKind()), [
                SyntaxKind.FunctionDeclaration,
            ]) &&
            !_.isEqual(declaration.map(x => x.getKind()), [
                SyntaxKind.FunctionDeclaration,
                SyntaxKind.Identifier,
            ])
        ) {
            console.error('Unexpected');
        }
        const exportAssignment = declaration[0];

        const nameTokens = file
            .getBaseNameWithoutExtension()
            .split('.');
        const name = nameTokens[nameTokens.length - 1];

        const oldText =
            exportAssignment.getPreviousSibling().getKind() ===
            SyntaxKind.SingleLineCommentTrivia
                ? exportAssignment.getText()
                : exportAssignment.getFullText();
        let newNode;
        if (oldText.includes('export default function')) {
            newNode = exportAssignment.replaceWithText(
                oldText.replace('export default', `export`),
            );
        } else {
            newNode = exportAssignment.replaceWithText(
                oldText.replace(
                    'export default',
                    `export const ${name} =`,
                ),
            );
        }
        
        // 3) Track references
        file.getReferencingSourceFiles().forEach(f => {
            results.push({
                exportFile: file,
                importFile: f,
                namedExport: { name },
            });
        });
    },
);

Note the acrobatics in order to define oldText. This is because getFullText() returns both SingleLineCommentTrivia and js doc comments, but SingleLineCommentTrivia exist as sibling nodes while the latter does not. So exportAssignment.replaceWithText(x) must define x to include js doc comments but exclude SingleLineCommentTrivia. This requires testing the previous sibling node. Is this desired? Am I doing it wrong?

Thanks!

Example Default Import 1

// tslint:disable-next-line:no-any only-arrow-functions
export default function Foo() {...}

Example Default Import 2

/** This is Foo */
export default function Foo() {...}

Example Default Import 3

 /**
  * Prevents an input's `onChange` property from firing on every change. Firing
  * on every change makes the input appear laggy. Instead it waits for changes
  * to stop happening for a set amount of time, then it triggers an `onChange`.
  */
 // tslint:disable-next-line:only-arrow-functions
export function Debounce<Val, BaseProps extends ControlledInput<Val>>(
     Component:
         | React.ComponentClass<BaseProps>
         | React.FunctionComponent<BaseProps>,
     ) {...}

[abirmingham]

Perhaps related to #178

[dsherret]

@abirmingham I'll read this and answer it sometime soon, but perhaps consider just doing the following:

if (functionDec.isDefaultExport()) {
     // will remove if it's default exported on a separate line
    functionDec.setIsDefaultExport(false);
    // Side note: I should have named this setIsNamedExport... will open an issue about that
    functionDeclaration.setIsExported(true);
}

Does doing that not work here?

[abirmingham]

@dsherret that did work! Thank you very much :)

[dsherret]

Just read this now. So when using replaceWithText you won't want to use getFullText(). getFullText() is the position from the compiler api position from the node's getPos() to getEnd()

See here: https://ts-ast-viewer.com/#code/GYVwdgxgLglg9mABFApgZygCgJSIN4CwAUMYmYgPQWICCwqATohADYwQDWMYA5sgBYpEYOABMhAIxQs4Ad1LkqiAG4wUsgUO7A4DALYBDWAkTdNiBjB78oCskoAOBsNORxEaFEKiCL6ECxQiHDAiAAGPChQAGIBLAAqKAAeUGEAdHbMbJwAEigMKADcxAC+QA

Instead, you'll want to use getText(true), which will include the js docs (this is all mirroring the compiler api... sorry that it's confusing). That uses the source file text from getStart(true) to getEnd(). In #178, it basically used to use getStart() (aka. getStart(false)) instead of getStart(true) for the start position.

I just ran this test:

const { Project } = require("ts-morph")

const project = new Project({ useVirtualFileSystem: true });

const sourceFile = project.createSourceFile("test1.ts", `// tslint:disable-next-line:no-any only-arrow-functions
export default function Foo() {}`);

const sourceFile2 = project.createSourceFile("test2.ts", `/** This is Foo */
export default function Foo() {}`);

const sourceFile3 = project.createSourceFile("test3.ts", ` /**
  * Comment
  */
 // tslint:disable-next-line:only-arrow-functions
export function Foo() {
}`);

doTest(sourceFile);
doTest(sourceFile2);
doTest(sourceFile3);

function doTest(sourceFile) {
    const func = sourceFile.getFunctions()[0];
    func.replaceWithText(func.getText(true));
    console.log(sourceFile.getFullText());
}

And it outputs the following as I would expect:

"// tslint:disable-next-line:no-any only-arrow-functions\nexport default function Foo() {}"
"/** This is Foo */\nexport default function Foo() {}"
" /**\n  * Comment\n  */\n // tslint:disable-next-line:only-arrow-functions\nexport function Foo() {\n}"

I've opened #725 to update the documentation to recommend using getText(true) when using replaceWithText.

But yes, please use the regular api for doing this (ex. setIsDefaultExport(false)). If you find yourself using replaceWithText and can't find any api for doing what you want to do, then please open an issue so I can make whatever it is easier for you 🙂

[abirmingham]

Thanks so much for the above-and-beyond explanation @dsherret ! I really appreciate that. I think that you see my confusion, but I wanted to give you a smaller example that illustrates how this can be confusing:

declare const node: Node;
node.replaceWithText(node.getFullText());

As a user I would expect this to be a no-op, but that only seems to be the case if there is no JSDoc comment. If there is a JSDoc comment, the comment is duplicated.

Anyway I think this smaller example is probably redundant at this point, as you understood my confusion and explained the reasoning well, but I wanted to add it because I realized that I could describe my original point of confusion with fewer words :)

Finally, I wonder if there should be a replaceWithFullText, or replaceWithText(x, true), but I'm too new to the library to judge. Just thinking out loud. Thanks again!

[dsherret]

As a user I would expect this to be a no-op

Yeah, it's slightly confusing, but that's what the compiler API does. In the compiler API and ts-morph it returns all the text from the last significant token of the previous node (comments not included) to the end of the node. So for example, calling getFullText() on the FunctionDeclaration node of myFunction would return the following:

getText() / getText(false) returns this:

Finally, getText(true) returns this:

As a user I would expect this to be a no-op, but that only seems to be the case if there is no JSDoc comment. If there is a JSDoc comment, the comment is duplicated.

Do you mean, the comment is duplicated? (not the jsdoc comment) Could you show an example where it duplicates the jsdoc comment? That might be a bug.

Finally, I wonder if there should be a replaceWithFullText, or replaceWithText(x, true), but I'm too new to the library to judge. Just thinking out loud. Thanks again!

getFullText() is not so useful. It's only really useful for getting the full source file text. Basically, just use getText(true) in this case.

[abirmingham]

@dsherret thanks again for the thoughtful response!

Do you mean, the comment is duplicated? (not the jsdoc comment) Could you show an example where it duplicates the jsdoc comment? That might be a bug.

Example

import { Project, SyntaxKind } from 'ts-morph';

const printWithSeperator = (header: string, str: string) => {
    console.log(`-- ${header} --`);
    console.log(str + `\n`);
};

const project = new Project();
const file = project.createSourceFile(
    'src/tmp.ts',
    [
        `// tslint:disable-next-line:no-any`,
        `const printName = (name: any) => console.log(name);`,
    ].join(`\n`),
);

// Print initial file
printWithSeperator('file.getFullText()', file.getFullText());

// Loop through children of SyntaxKind and print them (note dupe in output)
printWithSeperator(
    'file.getChildren()[0].getChildren()...',
    file
        .getChildren()[0]
        .getChildren()
        .map(x => x.getFullText())
        .join(', '),
);

// Get 'printName' variable statement
const functionDec = file.getFirstDescendantByKindOrThrow(
    SyntaxKind.VariableStatement,
);

// Replace functionDec with text of itself
functionDec.replaceWithText(functionDec.getFullText());

// Print final file (note dupe in output)
printWithSeperator('file.getFullText()', file.getFullText());

Prints

-- file.getFullText() --
// tslint:disable-next-line:no-any
const printName = (name: any) => console.log(name);

-- file.getChildren()[0].getChildren()... --
// tslint:disable-next-line:no-any, // tslint:disable-next-line:no-any
const printName = (name: any) => console.log(name);

-- file.getFullText() --
// tslint:disable-next-line:no-any
// tslint:disable-next-line:no-any
const printName = (name: string) => console.log(name);

Note the duplicate comment that appears in the second and third printed blocks. Again, I'll likely steer clear of getFullText() from now on, but this did behavior strike did me as odd.

Anyway, hope that helps!

[dsherret]

@abirmingham so in that scenario in the compiler api there is actually only one child that's the variable statement.

Using forEachChild(...) in the compiler api:

Then using getChildren():

https://ts-ast-viewer.com/#code/PTAEBcGcBsEsDtwC4AmtIEMBG0CmBaeXAD3HziKXgHt8N4BPAAwBoBYAKAGNr5JxQABwBOCcADkMAW1ygAvKAAU8abiSh6DAJTyAfKB59qeAHTRqAc2WqtAbiA (toggle the "tree mode" in the options)

Since comments are useful to know about for refactoring purposes, ts-morph superimposes certain kinds of comments so they appear in the results of getChildren() (but not forEachChild(...) and affect the result of getChildIndex(). They are also in getStatementsWithComments() and the insert index of methods like insertStatement(index, ...) will use the index of the items in getStatementsWithComments().

So in this case, calling getPos() on the variable statement returns 0 and calling getPos() on the "comment node" also returns 0 because it's superimposed. Read more here: https://ts-morph.com/details/comments

Generally I would recommend avoiding using getChildren() as well unless you need to know about tokens (ex. know about the position of an open brace or some keyword). Using forEachChild(...) is faster and better (same with in the compiler api). I should add that to the docs...

[abirmingham]

@dsherret sorry to keep pinging you on this, but I have a question that I think is related to comment nodes, and I'm not sure if it warrants opening up a new issue.

I'm writing a morph that aims to remove a selection of comments which disable the no-any tslint rule. The goal of the script is to identify cases where // tslint:disable-next-line:no-any occurs with no any on the subsequent line. My approach has been to use SourceFile#getDescendantsOfKind(SyntaxKind.SingleLineCommentTrivia) to identify the comment nodes, but this appears to be missing some cases.

Here is a simple reproduction:

import { Project, SyntaxKind } from 'ts-morph';

const project = new Project();
const file = project.createSourceFile(
    'src/tmp.ts',
    [
        `const sorted = [5, 3, 2, 7].sort(`,
        `    // tslint:disable-next-line:no-any`,
        `    (a, b) => {`,
        `        if (a === b) return 0;`,
        `        return a < b ? -1 : 1;`,
        `    },`,
        `);`,
    ].join(`\n`),
);

console.log(
    file.getDescendantsOfKind(SyntaxKind.SingleLineCommentTrivia).length,
); // 0

I also tried all of the other trivia kinds, SyntaxKind.NewLineTrivia. In this case it appears that the comment is not a node itself, and is only available as leading trivia on the ArrowFunction node, which makes it rather difficult to read/write to. I believe in this case I would need to use replaceWithText() in conjuction with getLeadingTriviaWidth() and getFullText(), which I'd prefer to avoid.

Note also that getText(true) on the ArrowFunction node doesn't appear to include the comment, whereas getFullText() does:

Any insight you could provide in order to accomplish this task would be greatly appreciated!

[dsherret]

@abirmingham sorry for not responding to this until now. By the way, I met a few of your colleagues on Friday! Hope you're feeling better this week.

ts-morph only parses out comments in the position of statements or members of something like an interface or class.

That said, after reading through what you wrote, I've opened up #737 to build on the work that was previously done and will probably pursue that. The current implementation is better than before, but I can definitely see how it's confusing and not so useful in your scenario. It also doesn't give people the ability to edit/remove those comments easily.

For now, you'll have to parse the comments out as if you were using the compiler API... basically, you'll need to call getLeadingCommentRanges() on every node and that will reparse out any comments each time (so it will be slow).

[abirmingham]

@dsherret thank you as always for the thoughtful response! #737 looks great and should solve the problem.

In the meantime, if I call getLeadingCommentRanges() to read the comments, what would you recommend I use to remove/edit the comments? I notice that there isn't setLeadingCommentRanges(). Note that in some cases I need to remove // tslint:disable-next-line:no-any, while in other cases I need to edit e.g. // tslint:disable-next-line:no-any deprecation to // tslint:disable-next-line:deprecation.

This isn't too urgent, so I may just wait for #737, but I am curious :)

Also I'm glad you were able to connect with my colleagues and I hope that TSConf was a positive experience! You're always more than welcome at the Seattle TS Meetup if you find yourself in the area.