Use NoInfer for better typing#1691
Conversation
|
@microsoft-github-policy-service agree |
| * | ||
| * @param handler The corresponding handler. | ||
| */ | ||
| onDocumentColor(handler: ServerRequestHandler<DocumentColorParams, ColorInformation[] | undefined | null, ColorInformation[], void>): Disposable; |
There was a problem hiding this comment.
Why did you remove undefined and null here?
There was a problem hiding this comment.
The reason for this is that result type specified here does not match result type, specified in DocumentColorRequest.type:
As you can see, result type does not allow null values. We should either add
null to DocumentColorRequest.type or remove null and undefined from Connection.onDocumentColor. Otherwise there will be a compilation error on line 1629. I have chosen the second option because it matches protocol specification.
vscode-languageserver-node/server/src/common/server.ts
Lines 1629 to 1631 in d522d4a
Same with onColorPresentation.
server/src/common/server.ts
Outdated
| * @param handler The corresponding handler. | ||
| */ | ||
| onColorPresentation(handler: ServerRequestHandler<ColorPresentationParams, ColorPresentation[] | undefined | null, ColorPresentation[], void>): Disposable; | ||
| onColorPresentation(handler: ServerRequestHandler<ColorPresentationParams, ColorPresentation[], ColorPresentation[], void>): Disposable; |
|
To keep things consistent with other requests that can return an array I would suggest the following:
|
| } | ||
|
|
||
| export type HandlerResult<R, E, _R = R extends null ? (R | undefined) : R> = _R | ResponseError<E> | Thenable<_R> | Thenable<ResponseError<E>> | Thenable<_R | ResponseError<E>>; | ||
| export type HandlerResult<R, E, _R = R extends null ? (R | undefined | void) : R> = _R | ResponseError<E> | Thenable<_R> | Thenable<ResponseError<E>> | Thenable<_R | ResponseError<E>>; |
There was a problem hiding this comment.
I made it possible to pass handlers that return void if null is acceptable, because handlers that return void will return undefined, and undefined will be automatically converted to null.
|
As suggested, I added I've also noticed that if parameter type allows export type RequestParam<P> = P extends null ? P | undefined : P;
// ...
sendRequest<P, R, PR, E, RO>(type: ProtocolRequestType<P, R, PR, E, RO>, params: NoInfer<RequestParam<P>>, token?: CancellationToken): Promise<R>;This change is necessary because before using Let me know what you think about this change. |
| } | ||
| } | ||
|
|
||
| export type RequestParam<P> = P extends null ? P | undefined : P; |
There was a problem hiding this comment.
RequestParam type is declared here.
|
I like the change of the |
|
/AzurePipelines run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
3 participants
Fixes #1685
After adding
NoInferto types of parameters, several compilation errors appeared in theConnectioninterface in the methods that register request handlers forDocumentLinkResolve,DocumentColor, andColorPresentation. The reason for them is that the return type of the request specified in those methods did not match the return type specified in the correspondingtypevariables and in the protocol specification. The return type does not allownullandundefined, so I removed those from the types.