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

Unable to rename a property to a PrivateIdentifier #1198

Closed
TimvdLippe opened this issue Sep 8, 2021 · 4 comments
Closed

Unable to rename a property to a PrivateIdentifier #1198

TimvdLippe opened this issue Sep 8, 2021 · 4 comments
Labels

Comments

@TimvdLippe
Copy link

TimvdLippe commented Sep 8, 2021

Describe the bug

Version: 12.0.0

To Reproduce

import { Project } from "ts-morph";

const project = new Project();
const sourceFile = project.createSourceFile("test.ts", `class Foo {
  private one: string;
  private two: string; 
}`);

const classNodes = sourceFile.getClasses();
for (const classNode of classNodes) {
  const baseClass = classNode.getBaseClass();
  for (const property of classNode.getInstanceProperties()) {
    const name = property.getStructure().name;
 
    if (name.startsWith('#') || !property.hasModifier(SyntaxKind.PrivateKeyword)) {
      continue;
    }

    property.toggleModifier('private', false);
    property.rename(`#${name}`, {
      renameInComments: true,
    });
  }
}

Expected behavior

A property that was private someProperty: string gets renamed to #someProperty:string

Actual behavior

ts-morph crashes when analyzing the second property to rename. E.g. it successfully renames the first property in a class, but then when it processes the second property in a class it fails with the following exception:

/Users/tvanderlippe/Projects/devtools/private-class-field-migration/node_modules/ts-morph/dist/ts-morph.js:2961
            throw new common.errors.InvalidOperationError(message);
            ^

InvalidOperationError: Attempted to get information from a node that was removed or forgotten.

Node text: private readonly secondProperty = this.onEventListener.bind(this);
    at PropertyDeclaration.get compilerNode [as compilerNode] (/Users/tvanderlippe/Projects/devtools/private-class-field-migration/node_modules/ts-morph/dist/ts-morph.js:2961:19)
    at PropertyDeclaration.getCompilerModifiers (/Users/tvanderlippe/Projects/devtools/private-class-field-migration/node_modules/ts-morph/dist/ts-morph.js:5994:25)
    at PropertyDeclaration.hasModifier (/Users/tvanderlippe/Projects/devtools/private-class-field-migration/node_modules/ts-morph/dist/ts-morph.js:5923:29)
    at Object.<anonymous> (/Users/tvanderlippe/Projects/devtools/private-class-field-migration/migrate.js:24:28)
    at Module._compile (internal/modules/cjs/loader.js:1063:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
    at Module.load (internal/modules/cjs/loader.js:928:32)
    at Function.Module._load (internal/modules/cjs/loader.js:769:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
    at internal/main/run_main_module.js:17:47

As a workaround, I currently use a specific prefix and then I manually replace SOME_UNIQUE_PREFIX with # in my editor.

property.rename(`SOME_UNIQUE_PREFIX_${name}`, {
  renameInComments: true,
});

Ideally property.rename can accept not just a string but also an Identifier Node that we can then pass in a PrivateIdentifier node.

@dsherret
Copy link
Owner

@TimvdLippe could you post some reproduction code? The source file in that example has empty text. Renaming to a private property works for me in a scenario I just tried out.

Also, it looks like what's failing in that example is that the property.hasModifier call. I think perhaps the rename() call is losing track of the property node somehow between renames. It is difficult to do all the time because that's using the language service and then getting the text changes under the hood and it tries to keep track of the nodes.

@TimvdLippe
Copy link
Author

@dsherret Ah apologies, I didn't know you could write the contents of the test file there. Running the above script fails with the following error:

private-class-field-migration/node_modules/ts-morph/dist/ts-morph.js:2961
            throw new common.errors.InvalidOperationError(message);
            ^

InvalidOperationError: Attempted to get information from a node that was removed or forgotten.

Node text: private two: string;
    at PropertyDeclaration.get compilerNode [as compilerNode] (private-class-field-migration/node_modules/ts-morph/dist/ts-morph.js:2961:19)
    at PropertyDeclaration.getCompilerModifiers (private-class-field-migration/node_modules/ts-morph/dist/ts-morph.js:5994:25)
    at PropertyDeclaration.hasModifier (private-class-field-migration/node_modules/ts-morph/dist/ts-morph.js:5923:29)
    at PropertyDeclaration.hasOverrideKeyword (private-class-field-migration/node_modules/ts-morph/dist/ts-morph.js:9350:25)
    at PropertyDeclaration.getStructure (private-class-field-migration/node_modules/ts-morph/dist/ts-morph.js:9370:42)
    at callBaseGetStructure (private-class-field-migration/node_modules/ts-morph/dist/ts-morph.js:537:51)
    at PropertyDeclaration.getStructure (private-class-field-migration/node_modules/ts-morph/dist/ts-morph.js:14400:16)
    at Object.<anonymous> (private-class-field-migration/migrate.js:14:27)
    at Module._compile (internal/modules/cjs/loader.js:1063:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)

@TimvdLippe
Copy link
Author

Oh, sorry if I was unclear. I updated the original issue comment with a reproduction case, where it uses 2 private identifiers where it crashes on the second refactoring.

@dsherret
Copy link
Owner

Thanks @TimvdLippe! This is definitely a bug. Will be fixed in the next release (I will probably do one today)

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

No branches or pull requests

2 participants