-
Notifications
You must be signed in to change notification settings - Fork 13k
More factory functions #15531
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
More factory functions #15531
Conversation
|
||
export function createKeywordTypeNode(kind: KeywordTypeNode["kind"]) { | ||
return <KeywordTypeNode>createSynthesizedNode(kind); | ||
export function createMethod(decorators: Decorator[] | undefined, modifiers: Modifier[] | undefined, asteriskToken: AsteriskToken | undefined, name: string | PropertyName, questionToken: QuestionToken | undefined, typeParameters: TypeParameterDeclaration[] | undefined, parameters: ParameterDeclaration[], type: TypeNode | undefined, body: Block | undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why make the Declaration
part of the name implicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consistency with most other factory functions. Most of the time we're transforming to JavaScript, and JavaScript doesn't have method signatures.
export function createParenthesizedType(type: TypeNode) { | ||
const node = <ParenthesizedTypeNode>createSynthesizedNode(SyntaxKind.ParenthesizedType); | ||
/* @internal */ | ||
export function createSignatureDeclaration(kind: SyntaxKind, typeParameters: TypeParameterDeclaration[] | undefined, parameters: ParameterDeclaration[], type: TypeNode | undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In analogue to the above, should this be createSignature
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per above, signatures are purely TypeScript. We can probably change the name if only to make it more concise.
|
||
export function createSignatureDeclaration(kind: SyntaxKind, typeParameters: TypeParameterDeclaration[] | undefined, parameters: ParameterDeclaration[], type: TypeNode | undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't really tell from the diff if the code was merely re-ordered or if there are more substantive changes. Looks similar on the LHS and RHS though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly reordered to align with the order in SyntaxKind
. It makes finding things easier.
This adds a number of missing factory functions for anyone writing custom transformers, as well as performs some cleanup in factory.ts.
Note, there are a few breaking API name changes in this PR as part of the public factory functions. I feel this is acceptable as custom transforms are still a fairly experimental feature, though if necessary we could export aliases for the renamed functions for backwards compatibility.
There's also a small change to the transform unit tests so that they aren't dependent on the new-line format of the host platform running the tests.
This builds on #15500 and #15511.