Skip to content

Refactor params for interface #42652

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

Merged

Conversation

jessetrinity
Copy link
Contributor

@jessetrinity jessetrinity commented Feb 4, 2021

Fixes #30418 for interfaces and type literals.

Cases for which we do not offer the refactor:

  • The type is a union or intersection.
  • There is more than one method signature.

}

/* Declarations in interface implementations have their own symbol so we need to go to the interface declaration
to get the type from the method signature. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand which part gets the type from the method signature. Is it entryToDeclaration? getSymbolForInterfaceSignature only gets the symbol.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed comment.

@jessetrinity jessetrinity requested a review from sandersn February 9, 2021 00:27
Comment on lines +57 to +61
if (signature) {
const newSignatureParams = map(createNewParameters(signature, program, host), param => getSynthesizedDeepClone(param));
replaceParameters(signature, newSignatureParams);
}
replaceParameters(functionDeclaration, newFunctionDeclarationParams);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these branches be mutually exclusive? This looks suspicious to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the signature is defined, that means we were able to connect the method declaration back to it. That is, functionDeclaration implements signature and both should be changed. The absence of signature simply means functionDeclaration does not implement a method declared elsewhere (that we know of).

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good enough to go in, although a test with unions might make sense if the code fix could be available.

////interface IBar {
//// method(x: number): void;
////}
////const x: IFoo & IBar = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens with unions? Do we even get the error that triggers the codefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a test refactorConvertParamsToDestructuredObject_interfaceNoUnion.ts:
https://github.com/microsoft/TypeScript/pull/42652/files#diff-bb37f20a7f9bacc80fda3468d1e6c808314c79f47e8f58dc3d55c7e3d5447360

It's a refactor so we just don't offer it.

@jessetrinity jessetrinity merged commit 1b19af0 into microsoft:master Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot run 'convert to named parameters' refactoring for method implementing an interface
4 participants