set exactOptionalPropertyTypes: true, improved types #725
Conversation
|
@ardatan please approve |
packages/server/test/fastify.spec.ts
Outdated
| @@ -35,6 +35,9 @@ describe('Fastify', () => { | |||
| url: '/mypath', | |||
| method: ['GET', 'POST', 'OPTIONS'], | |||
| handler: async (req, reply) => { | |||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | |||
| // @ts-ignore Types of property 'req' are incompatible. | |||
| // Type 'IncomingMessage' is missing the following properties from type 'Http2ServerRequest': authority, scheme, streamts(2345) | |||
There was a problem hiding this comment.
That's why we shouldn't use IncomingMessage or Http2ServerRequest interfaces directly.
There was a problem hiding this comment.
it is falling through from const rawRequest = nodeRequest.raw || nodeRequest.req || nodeRequest;
| req?: IncomingMessage | Http2ServerRequest | undefined; | ||
| raw?: IncomingMessage | Http2ServerRequest | undefined; |
There was a problem hiding this comment.
@ardatan This is where IncomingMessage type falls through into const rawRequest = nodeRequest.raw || nodeRequest.req || nodeRequest
| @@ -84,8 +93,12 @@ export function normalizeNodeRequest( | |||
| nodeRequest: NodeRequest, | |||
| RequestCtor: typeof Request, | |||
| ): Request { | |||
| // TODO: this is a main issue! | |||
| // const rawRequest: NodeRequest | IncomingMessage | Http2ServerRequest | |||
| const rawRequest = nodeRequest.raw || nodeRequest.req || nodeRequest; | |||
There was a problem hiding this comment.
const rawRequest: NodeRequest | Http2ServerRequest | IncomingMessage this is an issue, here you had created a union NodeRequest | Http2ServerRequest | IncomingMessage, and rawRequest is passed down into functions.
|
@ardatan can you please follow up? |
|
@ardatan can you please follow up this week? |
|
@JustFly1984 I'll try when I am available but I cannot merge this PR like that. It needs a lot of work. Current changes break the functionality, and I don't think it is a good trade to sacrifice all these functionalities just for this TS flag. |
|
@ardatan resolved merge conflicts |
|
@ardatan please follow up |
|
@ardatan please follow up. I really do not want to disable exactOptionalPropertyTypes in the project I have to use fets. |
|
@ardatan please follow up |
Description
Please include a summary of the change and which issue is fixed. Please also include relevant
motivation and context. List any dependencies that are required for this change.
Related #391
Type of change
Please delete options that are not relevant.
expected)
Screenshots/Sandbox (if appropriate/relevant):
Adding links to sandbox or providing screenshots can help us understand more about this PR and take
action on it as appropriate
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can
reproduce. Please also list any relevant details for your test configuration
Test Environment:
package-name:Checklist:
CONTRIBUTING doc and the
style guidelines of this project
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose
the solution you did and what alternatives you considered, etc...