Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add typescript definition #110

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ilikejames
Copy link

@ilikejames ilikejames commented Mar 28, 2023

A few years ago I had a quick attempt at adding types for this library. https://github.com/pillarjs/router/pull/76.

The http methods is copied from the methods library. I had hoped to use its types directly, but that proved tricky with building older versions of node, so instead it's a hard coded list copied from the library.

The types level tests are exhaustive, but let me know if there is anything I have missed.

There is a couple of assumptions made:

  • query parameters are always a string.
  • response is OutgoingMessage type.

I've limited the published types to typescript@4.0.0 and greater due to some of the syntax being specific to this more recent version.

I had wanted to include @types/node as a peerDependency but it appears that was not removable via npm rm X --save-peer and would result in a build error in early versions of npm

Edit: Made a further change. It looks like in the tests there is a lot of

  req.newProperty = 'new value'

To facilitate that, I've updated RoutedRequest (on the second commit).

This might be better done by supplying a generic, but that could be a further later addition.

Edit: Sorry for all the pushes/builds. Should have closed the PR, and worked on the fork only.

@ilikejames ilikejames force-pushed the add-typescript-definition branch 2 times, most recently from 2e63978 to 99dc6b0 Compare March 28, 2023 16:02
@ilikejames ilikejames marked this pull request as draft March 28, 2023 16:10
@ilikejames ilikejames force-pushed the add-typescript-definition branch 17 times, most recently from d31072f to c89a2d4 Compare March 29, 2023 21:30
@ilikejames ilikejames marked this pull request as ready for review March 29, 2023 21:40
@ilikejames ilikejames changed the title Add type definition Add typescript definition Mar 29, 2023
Comment on lines +6 to +32
type HttpMethods =
'get' |
'post' |
'put' |
'head' |
'delete' |
'options' |
'trace' |
'copy' |
'lock' |
'mkcol' |
'move' |
'purge' |
'propfind' |
'proppatch' |
'unlock' |
'report' |
'mkactivity' |
'checkout' |
'merge' |
'm-search' |
'notify' |
'subscribe' |
'unsubscribe' |
'patch' |
'search' |
'connect'
Copy link
Author

Choose a reason for hiding this comment

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

Take from methods lib

url?: string;
method?: string;
originalUrl?: string;
params?: Record<string, string>;
Copy link
Author

Choose a reason for hiding this comment

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

Assumption: params are always strings.

],
"typesVersions": {
">=4.0": {"*": ["index.d.ts"]}
Copy link
Author

Choose a reason for hiding this comment

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

Limited to downstream typescript@>4.0.0

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.

None yet

1 participant