JSDoc ?Type adds optionality to parameters#22646
Conversation
Chrome devtools expects that parameters with type `?T` (or `T?`) add null to `T` and optionality to the parameter. Previously it only added null to the type. Currently the PR does *not* add undefined to the type of `T`, which is expected by chrome-devtools-frontend, but is inconsistent with typescript's rules. The implementation achieves this inconsistency by exploiting the fact that checking the signature adds optionality and checking the parameter adds `undefined`.
| function isJSDocOptionalParameter(node: ParameterDeclaration) { | ||
| function isJSDocOptionalParameter(node: ParameterDeclaration, skipType?: boolean) { | ||
| return isInJavaScriptFile(node) && ( | ||
| node.type && node.type.kind === SyntaxKind.JSDocOptionalType |
There was a problem hiding this comment.
Might want to comment that this case is only for the parameters of a JSDocFunctionType, otherwise .type should always be TS syntax
Also, why does this one not look for JSDocNullableType too?
There was a problem hiding this comment.
Added. The comment. See @weswigham's report of our discussion for why skipType is incorrect — I'll push an update shortly.
| f(undefined) // should not be allowed | ||
| f(1) | ||
|
|
||
| /** @param {number?} a */ |
There was a problem hiding this comment.
This parses to the same node kind as ?number ?
| function g(a) { | ||
| a = 1 | ||
| a = null | ||
| a = undefined // should not be allowed |
There was a problem hiding this comment.
If we remove the assignments and return a; presumably that should include undefined, because a might not have been provided?
|
Just to remark on what we just discussed in person: On inspection, we're pretty sure the part making |
Previously isJSDocOptionalParameter was incorrect for types like `?number=`, which are optional but have JSDocNullableType as their root type node.
| isBracketed || !!typeExpression && skipJSDocPrefixTypes(typeExpression.type).kind === SyntaxKind.JSDocOptionalType)); | ||
| } | ||
|
|
||
| function skipJSDocPrefixTypes(type: TypeNode): TypeNode { |
There was a problem hiding this comment.
In ?!a=, maybe this should be parsed as (?!a)= and this wouldn't be necessary?
There was a problem hiding this comment.
I considered that and will look at that next. The reason is that, for ?T= the parser does
- parsePostfix
- parseNonArrayType
- parseJSDocNullableType
- parseType
- ...
- parsePostfix
parsePostfix at step (1) doesn't parse the trailing = in this case like we would want, the one at step (6) does. This might be fixed by changing step (4) to be parseNonArrayType. I'll try that and see what breaks.
There was a problem hiding this comment.
In fact, parseJSDocNodeWithType(SyntaxKind.JSDocNonNullable) does this already, so it's probably a mistake that parseJSDocNullableType doesn't.
There was a problem hiding this comment.
This breaks the existing parsing test in a way that I think is good (... is no longer allowed in the middle of a type), but I want to test more and maybe revise the parsing of ... too. I'll do that in a separate PR.
There was a problem hiding this comment.
Hmm, it also changes how tightly ?T binds -- if it's tighter than =, then it's tighter than [] too. So ?T[] is now (T | null)[] where before it was T[] | null.
chrome-devtools-frontend expects that parameters with type
?T(orT?) add null toTand optionality to the parameter. Previously it only added null to the type.Currently the PR does not add undefined to the type of
T, which is expected by chrome-devtools-frontend, but is inconsistent with typescript's rules. The implementation achieves this inconsistency by exploiting the fact that checking the signature adds optionality and checking the parameter addsundefined.