add refactor of convert private field to getter and setter#22143
add refactor of convert private field to getter and setter#221439 commits merged intomicrosoft:masterfrom
Conversation
38f3153 to
c632a1c
Compare
src/compiler/diagnosticMessages.json
Outdated
| "category": "Message", | ||
| "code": 95017 | ||
| }, | ||
| "Convert to getter and setter": { |
There was a problem hiding this comment.
"Generate 'get' and 'set' accessors."
You can potentially keep it as "getter" and "setter", but I don't know how well that localizes. I think the key thing I have in mind is that you should have the text "generate" in there somehow.
| function createGetter (fieldName: string, name: string, type: TypeNode) { | ||
| return createGetAccessor( | ||
| /*decorators*/ undefined, | ||
| [createToken(SyntaxKind.PublicKeyword)], |
There was a problem hiding this comment.
It'd be nice if we could detect whether other public members use modifiers instead of injecting one.
| function getConvertibleFieldAtPosition(file: SourceFile, startPosition: number): Info | undefined { | ||
| const node = getTokenAtPosition(file, startPosition, /*includeJsDocComment*/ false); | ||
| const propertyDeclaration = findAncestor(node.parent, isPropertyDeclaration); | ||
| if (!(propertyDeclaration && propertyDeclaration.name.getText().charAt(0) === "_" && hasModifier(propertyDeclaration, ModifierFlags.Private))) return undefined; |
There was a problem hiding this comment.
Give a short comment explaining why you want to do this check.
Also, use charCodeAt(0) === CharacterCodes._.
Also, break this into at least two lines and use braces please.
There was a problem hiding this comment.
Also, does this work for computed properties? You probably want to make sure that your propertyDeclaration.name is an identifier. That way you could also use propertyDeclaration.name.text after you use a type-guard.
There was a problem hiding this comment.
I don't like that you have to write private _foo: number; to get this refactor. Wouldn't it make more sense to trigger on public foo: number; and convert it to private _foo: number; public get foo() {} public set foo() {}?
| function getConvertibleFieldAtPosition(file: SourceFile, startPosition: number): Info | undefined { | ||
| const node = getTokenAtPosition(file, startPosition, /*includeJsDocComment*/ false); | ||
| const propertyDeclaration = findAncestor(node.parent, isPropertyDeclaration); | ||
| if (!(propertyDeclaration && propertyDeclaration.name.getText().charAt(0) === "_" && hasModifier(propertyDeclaration, ModifierFlags.Private))) return undefined; |
There was a problem hiding this comment.
Also, does this work for computed properties? You probably want to make sure that your propertyDeclaration.name is an identifier. That way you could also use propertyDeclaration.name.text after you use a type-guard.
|
|
||
| return { | ||
| fieldName: propertyDeclaration.name.getText(), | ||
| name: propertyDeclaration.name.getText().substring(1), |
There was a problem hiding this comment.
You will actually need to make sure that you create a unique name that doesn't exist in the current class. For simplicity, I would just bail out if the property with this name already exists.
| if (!(propertyDeclaration && propertyDeclaration.name.getText().charAt(0) === "_" && hasModifier(propertyDeclaration, ModifierFlags.Private))) return undefined; | ||
|
|
||
| return { | ||
| fieldName: propertyDeclaration.name.getText(), |
There was a problem hiding this comment.
Same deal with propertyDeclaration.name.text here and below.
c632a1c to
95c3ca5
Compare
| actionDescription: "Generate 'get' and 'set' accessors", | ||
| newContent: `class A { | ||
| protected _a: string; | ||
| public get a(): string { |
There was a problem hiding this comment.
If the initial property access is protected then this should be protected. The inner variable should be private
src/services/refactors/refactors.ts
Outdated
| /// <reference path="extractSymbol.ts" /> | ||
| /// <reference path="installTypesForPackage.ts" /> | ||
| /// <reference path="useDefaultImport.ts" /> | ||
| /// <reference path="ConvertToGetterAndSetter.ts" /> |
There was a problem hiding this comment.
Nit: please keep file casing consistent by renaming the file and this reference to it. (first letter should be lower-case.
DanielRosenwasser
left a comment
There was a problem hiding this comment.
Getting close! Make sure you also have tests for a
staticmember (maybe don't provide any quickfix here)readonlymember (maybe only generate aget-accessor, not clear if you want to keep thereadonlymodifier around)
| ); | ||
| } | ||
|
|
||
| function updateOriginPropertyDeclaration (propertyDeclaration: PropertyDeclaration, fieldName: string, modifiers: ModifiersArray) { |
There was a problem hiding this comment.
updateoriginalPropertyDeclaration
| function updateOriginPropertyDeclaration (propertyDeclaration: PropertyDeclaration, fieldName: string, modifiers: ModifiersArray) { | ||
| return updateProperty( | ||
| propertyDeclaration, | ||
| /*decorators*/ undefined, |
There was a problem hiding this comment.
Seems problematic to drop decorators here.
| /*decorators*/ undefined, | ||
| modifiers, | ||
| fieldName, | ||
| /*questionOrExclamationToken*/ undefined, |
There was a problem hiding this comment.
Also strange to drop the token.
| }; | ||
| } | ||
|
|
||
| interface Info { originName: string; fieldName: string; accessorName: string; propertyDeclaration: PropertyDeclaration; needUpdateName: boolean; hasModifiers: boolean; needUpdateModifiers: boolean; } |
There was a problem hiding this comment.
originName to originalName
There was a problem hiding this comment.
Put this on multiple lines.
| //// /*a*/public a: string;/*b*/ | ||
| //// } | ||
|
|
||
| goTo.select("a", "b"); |
There was a problem hiding this comment.
I believe we have a range API instead, but I haven't ever written one of these tests yet, so this might be fine?
There was a problem hiding this comment.
i'm not sure...
I copied it from the refactor Convert Es6 Module
|
|
||
| if (find(members, member => needUpdateName ? member.name.getText() === fieldName : member.name.getText() === accessorName)) return undefined; | ||
|
|
||
| const hasModifiers = !!find(members, member => !!member.modifiers); |
There was a problem hiding this comment.
Hmm, I think I initially meant whether a field had a public modifier explicitly
There was a problem hiding this comment.
what if the field is Protected
95c3ca5 to
11637de
Compare
|
After thinking, I tend to not support 1. Initializer is not a Literal (may be side effects)it cannot be convert as Literal 2.before: after: it can mod or: it seems do nothing |
ba0f71b to
e39a889
Compare
dcdb450 to
9acfc4c
Compare
| //// } | ||
|
|
||
| goTo.select("a", "b"); | ||
| verify.not.refactorAvailable("Generate 'get' and 'set' accessors"); No newline at end of file |
There was a problem hiding this comment.
i do not see why we would not allow this. you just need to come up with a temporary name that starts with _a, set the rename location to it, and let the user decide what they want to call it. you can just call createUniqueName to create you an identifier with a unique name and use it instead.
| actionDescription: "Generate 'get' and 'set' accessors", | ||
| newContent: `class A { | ||
| _a: string; | ||
| get a(): string { |
There was a problem hiding this comment.
why? i would expect this to be get _a
| if (!fieldInfo) return undefined; | ||
|
|
||
| const changeTracker = textChanges.ChangeTracker.fromContext(context); | ||
| const newLineCharacter = getNewLineOrDefaultFromHost(context.host, context.formatContext.options); |
There was a problem hiding this comment.
newLineCharacter should now be available on ChangeTracker.newLineCharacter , no need to recompute it,
| const propertyDeclaration = findAncestor(node.parent, isPropertyDeclaration); | ||
|
|
||
| if (!propertyDeclaration || propertyDeclaration.name.kind !== SyntaxKind.Identifier) return undefined; | ||
| // make sure propertyDeclaration have only AccessibilityModifier |
There was a problem hiding this comment.
what about static? you can have static accessors
|
|
||
| function getConvertibleFieldAtPosition(file: SourceFile, startPosition: number): Info | undefined { | ||
| const node = getTokenAtPosition(file, startPosition, /*includeJsDocComment*/ false); | ||
| const propertyDeclaration = findAncestor(node.parent, isPropertyDeclaration); |
There was a problem hiding this comment.
we also need to handle parameter properties. so this can be a property or a parameter declaration.
| // make sure propertyDeclaration have only AccessibilityModifier | ||
| if ((getModifierFlags(propertyDeclaration) | ModifierFlags.AccessibilityModifier) !== ModifierFlags.AccessibilityModifier) return undefined; | ||
|
|
||
| const containerClass = getContainingClass(propertyDeclaration); |
There was a problem hiding this comment.
instead of these two lines, you can just check that isClassLike(propertyDeclaration.parent) or isClassLike(parameterDeclaration.parent.parent)
| const members = getMembersOfDeclaration(containerClass); | ||
| if (!members) return undefined; | ||
|
|
||
| const needUpdateName = propertyDeclaration.name.text.charCodeAt(0) !== CharacterCodes._; |
There was a problem hiding this comment.
do not think you need that. as i noted in my other comment, we should pick a unique name that has a seed of "_" + <property name>, then set the rename location on this name. the user will get the chance to rename it right there and then.
There was a problem hiding this comment.
so in short the name should always be updated.
| const accessorName = needUpdateName ? propertyDeclaration.name.text : propertyDeclaration.name.text.substring(1); | ||
| const fieldName = `_${accessorName}`; | ||
|
|
||
| if (find(members, member => needUpdateName ? member.name.getText() === fieldName : member.name.getText() === accessorName)) return undefined; |
There was a problem hiding this comment.
do not think you need these either.
|
|
||
| const hasModifiers = !!find(members, member => !!member.modifiers); | ||
| const needUpdateModifiers = hasModifiers && (!propertyDeclaration.modifiers || !hasModifier(propertyDeclaration, ModifierFlags.Private)); | ||
| const accessorType = propertyDeclaration.questionToken ? mergeTypeNodeToUnion(propertyDeclaration.type, createKeywordTypeNode(SyntaxKind.UndefinedKeyword)) : propertyDeclaration.type; |
There was a problem hiding this comment.
i would say you do not need to create this type until you need to execute the refactoring. this function is called in both getAvailableActions and getEditsForAction, so ideally we would defer that until we are computing the edits.
| const fieldInfo = getConvertibleFieldAtPosition(file, startPosition); | ||
| if (!fieldInfo) return undefined; | ||
|
|
||
| return [ |
There was a problem hiding this comment.
nit, keep the [ and { on the same line.
| const hasModifiers = !!find(members, member => !!member.modifiers); | ||
| const needUpdateModifiers = hasModifiers && (!propertyDeclaration.modifiers || !hasModifier(propertyDeclaration, ModifierFlags.Private)); | ||
| const accessorType = propertyDeclaration.questionToken ? mergeTypeNodeToUnion(propertyDeclaration.type, createKeywordTypeNode(SyntaxKind.UndefinedKeyword)) : propertyDeclaration.type; | ||
|
|
There was a problem hiding this comment.
also keep in mind that propertyDeclaration.type may not be there. if this is the case, you want to not include a type, nor do you want to merge it with undefined.
also you should add a test for that.
mhegazy
left a comment
There was a problem hiding this comment.
Looks good, a few comments. also please add a test for applying the refactoring on a .js file
| function getAvailableActions(context: RefactorContext): ApplicableRefactorInfo[] | undefined { | ||
| const { file, startPosition } = context; | ||
|
|
||
| const fieldInfo = getConvertibleFieldAtPosition(file, startPosition); |
| const accessorName = needUpdateName ? propertyDeclaration.name.text : propertyDeclaration.name.text.substring(1); | ||
| const fieldName = `_${accessorName}`; | ||
|
|
||
| if (find(members, member => needUpdateName ? member.name.getText() === fieldName : member.name.getText() === accessorName)) return undefined; |
There was a problem hiding this comment.
Prefer members.some(...) over find when just using as a boolean
There was a problem hiding this comment.
Also, .getText() on every member might be slow since getText() has to do scanning. Might want to do something like function getDeclarationName from services.ts to just get Identifier and StringLiteral-named properties.
There was a problem hiding this comment.
Also, getMemberName(member.name) === (needUpdateName ? fieldName : accessorName)
There was a problem hiding this comment.
Actually, can avoid looping over members and instead check for checker.getPropertyOfType(checker.getTypeAtLocation(containerClass.name), needUpdateName ? fieldName : accessorName) which avoids conflicts with supertypes too.
| const containerClass = getContainingClass(propertyDeclaration); | ||
| if (!containerClass) return undefined; | ||
|
|
||
| const members = getMembersOfDeclaration(containerClass); |
| const newLineCharacter = getNewLineOrDefaultFromHost(context.host, context.formatContext.options); | ||
|
|
||
| const { fieldName, accessorName, accessorType, propertyDeclaration, needUpdateName, hasModifiers, needUpdateModifiers } = fieldInfo; | ||
| const accessorModifiers = hasModifiers ? ( |
There was a problem hiding this comment.
hasModifiers currently is true if any member on the class has modifiers. Instead this could just preserve whatever modifier was there before on the particular member without looking at other members. I.e., public x: number becomes public get x() { ... } public set x() { ... } and x: number becomes get x() { ... } set x() { ... }, making hasModifiers unnecessary.
| const changeTracker = textChanges.ChangeTracker.fromContext(context); | ||
| const newLineCharacter = getNewLineOrDefaultFromHost(context.host, context.formatContext.options); | ||
|
|
||
| const { fieldName, accessorName, accessorType, propertyDeclaration, needUpdateName, hasModifiers, needUpdateModifiers } = fieldInfo; |
There was a problem hiding this comment.
needUpdateName and needUpdateModifiers aren't used separately, so I would combine them to one property.
| const fieldInfo = getConvertibleFieldAtPosition(file, startPosition); | ||
| if (!fieldInfo) return undefined; | ||
|
|
||
| const changeTracker = textChanges.ChangeTracker.fromContext(context); |
| const modifiers = hasModifiers ? createNodeArray([createToken(SyntaxKind.PrivateKeyword)]) : undefined; | ||
| if (needUpdateName || needUpdateModifiers) { | ||
| changeTracker.replaceNode(file, propertyDeclaration, updateoriginalPropertyDeclaration(propertyDeclaration, fieldName, modifiers), { | ||
| suffix: newLineCharacter |
There was a problem hiding this comment.
With the latest changes to ChangeTracker it shouldn't be necessary to explicitly specify options. If it is, file an issue and we can fix it later after this PR is merged.
| name, | ||
| [createParameter( | ||
| /*decorators*/ undefined, | ||
| /*modifies*/ undefined, |
| const accessorType = propertyDeclaration.questionToken ? mergeTypeNodeToUnion(propertyDeclaration.type, createKeywordTypeNode(SyntaxKind.UndefinedKeyword)) : propertyDeclaration.type; | ||
|
|
||
| return { | ||
| originalName: propertyDeclaration.name.text, |
There was a problem hiding this comment.
Might want to make const originalName early since it's used in 3 other places.
|
@mhegazy |
we do allow all "standard-track" features in JS: |
|
@Kingwl i think we should support class C {
readonly x = 0;
}should convert to class C {
private _x = 0;
get x() {
return this._x;
}
}I think we should do that in a separate PR though, since there is some complications with constructor access here.. you want to replace all references to the class C {
readonly x: number;
constructor() {
this.x = 0;
}
}should become: class C {
private _x: number;
get x() {
return this._x;
}
constructor () {
this._x = 0; // _x and not x here
}
} |
|
gave it a quick test, found two issues..
/** class comment */
class C {
// Field commment
x = 22;
}as i said before, we should look into the |
|
Sure, but it might be late. I'm enjoying my holiday in Disney(three days)😁 |
|
no worries. take your time. and have a great vacation °o° |
|
ahhhh, could refactor apply with |
i do not see why not. good point. |
src/services/textChanges.ts
Outdated
| return { prefix: ", " }; | ||
| } | ||
| else if (isPropertyAssignment(node)) { | ||
| return { suffix: "," + this.newLineCharacter } |
There was a problem hiding this comment.
@mhegazy some thing need help
in this case
how should i add the , after every props
if i update the ObjectLiteralExpression, the original PropertyAssignment will overship with this change
| function getPropertyAssignmentDeclarationInfo(propertyAssignment: PropertyAssignment): DeclarationInfo | undefined { | ||
| return { | ||
| isStatic: false, | ||
| type: undefined, |
There was a problem hiding this comment.
what should i do with this type declaration,
should i get the type with typeChecker?
There was a problem hiding this comment.
no, you are fine. the type will be inferred from the return type of the getter.
|
|
||
| function getFieldModifiers(isStatic: boolean): NodeArray<Modifier> { | ||
| function getFieldModifiers(isJS: boolean, isStatic: boolean, isClassLike: boolean): NodeArray<Modifier> | undefined { | ||
| if (isJS || !isClassLike) return undefined; |
There was a problem hiding this comment.
you still want to handel the static modifier in the case of js. i would jsut make it
return createNodeArray(
append<Modifier>(isJS ? [] : [createToken(SyntaxKind.PrivateKeyword)],
isStatic ? createToken(SyntaxKind.StaticKeyword) : undefined));|
@Andy-MS any more comments? |
|
thanks @Kingwl!. wanna get the readonly support in next? |
|
sure 😅 |
|
wait for pr been merged which @Andy-MS has send and thanks for the review 😬 |
|
Go for it. |

Fixes #12417