Use alias symbol for optional parameter declaration serialization #42170
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
| @@ -5823,6 +5823,20 @@ namespace ts { | |||
| return !(getObjectFlags(type) & ObjectFlags.Reference) || !isTypeReferenceNode(existing) || length(existing.typeArguments) >= getMinTypeArgumentCount((type as TypeReference).target.typeParameters); | |||
| } | |||
|
|
|||
| function serializeParameterTypeForDeclaration(context: NodeBuilderContext, type: Type, symbol: Symbol, enclosingDeclaration: Node | undefined, includePrivateSymbol?: (s: Symbol) => void, bundled?: boolean) { | |||
| if (type !== errorType && symbol.valueDeclaration?.kind === SyntaxKind.Parameter && (symbol.valueDeclaration as ParameterDeclaration).questionToken) { | |||
There was a problem hiding this comment.
Do we have to look at the value declaration here? Or is there a (derived) CheckFlag on the symbol which indicates optionality? Moreover, do we already have an isOptionalParameterSymbol helper? I can't remember offhand.
|
#42149 seems potentially related here. |
| context.approximateLength += 12; // " | undefined" | ||
| return factory.createUnionTypeNode([ | ||
| typeToTypeNodeHelper(nonOptionalType, context), | ||
| factory.createKeywordTypeNode(SyntaxKind.UndefinedKeyword) |
There was a problem hiding this comment.
Do we need to union with undefined? I thought the nullable ? already implies that.
There was a problem hiding this comment.
Declaration emit (or any kind of type printback) always expands optional parameters/properties to their union type annotation, so this is just to match the convention. In some tangent on some other PR I floated the idea of reversing that decision for printback in errors and quick info to make it more succinct, but Anders wasn't keen on it.
At any rate, I'm pretty sure this PR is redundant with #42149. Now that it’s merged, have you tried your test case with it?
There was a problem hiding this comment.
Hm - I have always kind of hated the extra | undefined unless we've lost the context where it came from.
There was a problem hiding this comment.
I think it’s something we should revisit sometime.
|
Superseded by #42149 |
Fixes #42165