Skip to content

Add TypeScript typings#39

Merged
mcollina merged 3 commits intofastify:masterfrom
Ethan-Arrowood:feat/typescript-d-file
Apr 4, 2019
Merged

Add TypeScript typings#39
mcollina merged 3 commits intofastify:masterfrom
Ethan-Arrowood:feat/typescript-d-file

Conversation

@Ethan-Arrowood
Copy link
Copy Markdown
Member

Spent some time trying to complete this tonight. Please review as accurately as possible. All of my types are from reading the code. I based some off of the JSON Schema object but also included anything written in the documentation too. There were a couple any types I'd like to see if we can make more specific, as well as some general questions I have:

  • Is the port property on the InjectOptions.url type of string | number?
  • What is query? Could this be typed as string?
  • What types can payload be? I've implemented it as a generic for now but I think we can scope this to an union type list: string | object | Buffer | NodeJS.ReadableStream (this is actually copied from the current fastify.d.ts types HTTPInjectOptions so I believe it should be safe to implement here as well). Depending on what happens here I'll most likely be removing the generic.
  • Am I correct that Response._lightMyRequest.payloadChunks is of type Array<Buffer>?

Not ready to be merged yet as I'm still playing with tsd to test the new types. It is a young module with a tiny API so it might not be quite ready yet, but I'd like to revisit it with a clear head tomorrow.

Finally, I need to experiment with the declaration file exports, namespace, and some of the internal TSC stuff I used (such as interface) to make sure it is correct.

Comment thread package.json Outdated
Comment thread index.d.ts Outdated
Comment thread index.d.ts Outdated
Comment thread index.d.ts
Comment thread index.d.ts Outdated
Comment thread index.d.ts Outdated
Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcollina mcollina requested a review from delvedor March 26, 2019 12:04
Comment thread tsconfig.json Outdated
@Ethan-Arrowood
Copy link
Copy Markdown
Member Author

Pinging @delvedor for review in case the notification was missed

@mcollina mcollina merged commit f9b2a96 into fastify:master Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants