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 files #100

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,27 +34,32 @@ jobs:
- name: Node.js 0.8
node-version: "0.8"
npm-i: [email protected] [email protected]
npm-rm: nyc
npm-rm: nyc typescript @types/node

- name: Node.js 0.10
node-version: "0.10"
npm-i: [email protected] [email protected] [email protected]
npm-rm: typescript @types/node

- name: Node.js 0.12
node-version: "0.12"
npm-i: [email protected] [email protected] [email protected]
npm-rm: typescript @types/node

- name: io.js 1.x
node-version: "1.8"
npm-i: [email protected] [email protected] [email protected]
npm-rm: typescript @types/node

- name: io.js 2.x
node-version: "2.5"
npm-i: [email protected] [email protected] [email protected]
npm-rm: typescript @types/node

- name: io.js 3.x
node-version: "3.3"
npm-i: [email protected] [email protected] [email protected]
npm-rm: typescript @types/node

- name: Node.js 4.x
node-version: "4.9"
Expand Down Expand Up @@ -161,6 +166,10 @@ jobs:
if: steps.list_env.outputs.eslint != ''
run: npm run lint

- name: Test types
if: steps.list_env.outputs.typescript != ''
run: npm run test-types

- name: Collect code coverage
uses: coverallsapp/github-action@master
if: steps.list_env.outputs.nyc != ''
Expand Down
193 changes: 193 additions & 0 deletions index.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
import * as http from 'http';

export = Router;

declare namespace Router {

interface RouterOptions {
strict?: boolean;
caseSensitive?: boolean;
mergeParams?: boolean;
}

interface Layer {
name: string,
handle: RequestHandler | ErrorRequestHandler,
handle_request: RequestHandler,
handle_error: ErrorRequestHandler,
match: (path: string) => boolean,
}

interface IncomingRequest extends http.IncomingMessage {
originalUrl?: string,
params?: {
[key: string]: any,
},
}

interface RoutedRequest extends IncomingRequest {
baseUrl: string,
next?: NextFunction,
route?: IRoute
}

type RequestParamHandler = (req: IncomingRequest, res: http.ServerResponse, next: NextFunction, value: string, name: string) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to figure out where it went wrong in the TS, and I think this line is the culprit: inside of the router.param callback, there is req.params, req.route, and all the other things available. I think that this should be the "routed request" type here.

Copy link
Author

Choose a reason for hiding this comment

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

Changed that to RoutedRequest.


type RouteHandler = (req: RoutedRequest, res: http.ServerResponse, next: NextFunction) => void;
type RequestHandler = (req: IncomingRequest, res: http.ServerResponse, next: NextFunction) => void;

type NextFunction = (err?: Error | "route" | "router") => void;
type Callback = (err?: Error) => void;

type ErrorRequestHandler = (err: Error, req: IncomingRequest, res: http.ServerResponse, next: NextFunction) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

We may not want the err to be typed as just a Error object, as in the 1.x of this router, there is no enforcement on that; using a middleware you didn't write could have a string or plain object here, which I've seen a lot. We wouldn't want someone's TS code to think they don't need some kind of guard on the err value.

Copy link
Author

@rauno56 rauno56 Jun 29, 2021

Choose a reason for hiding this comment

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

What's the more appropriate type for that then? any?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is probably fine. It does have to be truthy, though, so not sure if that helps at all.

Copy link
Author

Choose a reason for hiding this comment

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

Changed it to any.


type PathParams = string | RegExp | Array<string | RegExp>;

type RequestHandlerParams = RouteHandler | ErrorRequestHandler | Array<RouteHandler | ErrorRequestHandler>;

interface IRouterMatcher<T> {
(path: PathParams, ...handlers: RouteHandler[]): T;
(path: PathParams, ...handlers: RequestHandlerParams[]): T;
}

interface IRouterHandler<T> {
(...handlers: RouteHandler[]): T;
(...handlers: RequestHandlerParams[]): T;
}

interface IRouter {
/**
* Map the given param placeholder `name`(s) to the given callback(s).
*
* Parameter mapping is used to provide pre-conditions to routes
* which use normalized placeholders. For example a _:user_id_ parameter
* could automatically load a user's information from the database without
* any additional code,
*
* The callback uses the same signature as middleware, the only differencing
* being that the value of the placeholder is passed, in this case the _id_
* of the user. Once the `next()` function is invoked, just like middleware
* it will continue on to execute the route, or subsequent parameter functions.
*
* app.param('user_id', function(req, res, next, id){
* User.find(id, function(err, user){
* if (err) {
* next(err);
* } else if (user) {
* req.user = user;
* next();
* } else {
* next(new Error('failed to load user'));
* }
* });
* });
*/
param(name: string, handler: RequestParamHandler): this;

/**
* Special-cased "all" method, applying the given route `path`,
* middleware, and callback to _every_ HTTP method.
*/
all: IRouterMatcher<this>;

use: IRouterHandler<this> & IRouterMatcher<this>;

handle: (req: http.IncomingMessage, res: http.ServerResponse, cb: Callback) => void;

route(prefix: PathParams): IRoute;
// Stack of configured routes
stack: Layer[];
Copy link
Contributor

Choose a reason for hiding this comment

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

As a general question, are the typings supposed to include all properties and methods including private ones, so should this be removed since it is not a public property?

Copy link
Author

Choose a reason for hiding this comment

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

You are right. API documented private in the code should be defined private in TS as well. I didn't pay much attention to that since

  • "it's JS - accessible anyways!" and
  • I've actually needed to access those myself.

I'll go over it and add access modifiers where appropriate next week.

Copy link
Author

Choose a reason for hiding this comment

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

@dougwilson, I looked into it a bit, but don't know a good way to implement it. I'd leave it as it is for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume the way to implement is to not have it in here at all. My understanding (which is weak, please correct me if wrong) is that the TSD is just the public API, which is why there is no mechanism to mark something as private.

Copy link
Author

Choose a reason for hiding this comment

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

Commented out handle() and stack - left Layer type because it's useful for documenting the internal API, which will be inaccessible nevertheless.


// Common HTTP methods
delete: IRouterMatcher<this>;
get: IRouterMatcher<this>;
head: IRouterMatcher<this>;
options: IRouterMatcher<this>;
patch: IRouterMatcher<this>;
post: IRouterMatcher<this>;
put: IRouterMatcher<this>;

// Exotic HTTP methods
acl: IRouterMatcher<this>;
bind: IRouterMatcher<this>;
checkout: IRouterMatcher<this>;
connect: IRouterMatcher<this>;
copy: IRouterMatcher<this>;
link: IRouterMatcher<this>;
lock: IRouterMatcher<this>;
"m-search": IRouterMatcher<this>;
merge: IRouterMatcher<this>;
mkactivity: IRouterMatcher<this>;
mkcalendar: IRouterMatcher<this>;
mkcol: IRouterMatcher<this>;
move: IRouterMatcher<this>;
notify: IRouterMatcher<this>;
pri: IRouterMatcher<this>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this actually a method on there?

$ node -pe 'require(".")().pri'
undefined

Copy link
Author

@rauno56 rauno56 May 16, 2021

Choose a reason for hiding this comment

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

I took that from require('http').METHODS. v14.16.1 has it at least.

Now... this is not ideal obviously and I'm sure there's a way to handle supporting different versions better, but my current approach has been "close is better than nothing".

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, gotcha. I was using Node.js 16.1. So that does bring up a good point: I guess this should be all the methods possible in the various Node.js versions? I wonder what other methods got removed; probably should go through all the versions this module supports and union them together.

Copy link
Author

Choose a reason for hiding this comment

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

I asked around in TS discord and they didn't recall a better way to handle it than what you proposed - adding collecting all of them from all versions.

propfind: IRouterMatcher<this>;
proppatch: IRouterMatcher<this>;
purge: IRouterMatcher<this>;
rebind: IRouterMatcher<this>;
report: IRouterMatcher<this>;
search: IRouterMatcher<this>;
source: IRouterMatcher<this>;
subscribe: IRouterMatcher<this>;
trace: IRouterMatcher<this>;
unbind: IRouterMatcher<this>;
unlink: IRouterMatcher<this>;
unlock: IRouterMatcher<this>;
unsubscribe: IRouterMatcher<this>;
}

interface IRoute {
path: string;
stack: Layer[];

all: IRouterHandler<this>;

// Common HTTP methods
delete: IRouterHandler<this>;
get: IRouterHandler<this>;
head: IRouterHandler<this>;
options: IRouterHandler<this>;
patch: IRouterHandler<this>;
post: IRouterHandler<this>;
put: IRouterHandler<this>;

// Exotic HTTP methods
acl: IRouterHandler<this>;
bind: IRouterHandler<this>;
checkout: IRouterHandler<this>;
connect: IRouterHandler<this>;
copy: IRouterHandler<this>;
link: IRouterHandler<this>;
lock: IRouterHandler<this>;
"m-search": IRouterHandler<this>;
merge: IRouterHandler<this>;
mkactivity: IRouterHandler<this>;
mkcalendar: IRouterHandler<this>;
mkcol: IRouterHandler<this>;
move: IRouterHandler<this>;
notify: IRouterHandler<this>;
pri: IRouterHandler<this>;
propfind: IRouterHandler<this>;
proppatch: IRouterHandler<this>;
purge: IRouterHandler<this>;
rebind: IRouterHandler<this>;
report: IRouterHandler<this>;
search: IRouterHandler<this>;
source: IRouterHandler<this>;
subscribe: IRouterHandler<this>;
trace: IRouterHandler<this>;
unbind: IRouterHandler<this>;
unlink: IRouterHandler<this>;
unlock: IRouterHandler<this>;
unsubscribe: IRouterHandler<this>;
}

interface RouterConstructor extends IRouter {
new(options?: RouterOptions): IRouter & ((req: http.IncomingMessage, res: http.ServerResponse, cb: Callback) => void);
}

}

declare const Router: Router.RouterConstructor;
7 changes: 6 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,26 @@
"utils-merge": "1.0.1"
},
"devDependencies": {
"@types/node": "^10.0.0",
"after": "0.8.2",
"eslint": "7.26.0",
"eslint-plugin-markdown": "2.1.0",
"finalhandler": "1.1.2",
"mocha": "8.4.0",
"nyc": "15.1.0",
"safe-buffer": "5.2.1",
"supertest": "6.1.3"
"supertest": "6.1.3",
"typescript": "^4.2.4"
},
"files": [
"lib/",
"LICENSE",
"HISTORY.md",
"README.md",
"index.d.ts",
"index.js"
],
"types": "index.d.ts",
"engines": {
"node": ">= 0.8"
},
Expand All @@ -42,6 +46,7 @@
"test": "mocha --reporter spec --bail --check-leaks test/",
"test-ci": "nyc --reporter=lcov --reporter=text npm test",
"test-cov": "nyc --reporter=text npm test",
"test-types": "tsc --project tsconfig.json --noEmit",
"version": "node scripts/version-history.js && git add HISTORY.md"
}
}
66 changes: 66 additions & 0 deletions test/type-check.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import { createServer, IncomingMessage, ServerResponse } from 'http';
import {
default as Router,
RouterOptions,
IncomingRequest,
RouteHandler,
IRoute,
RoutedRequest
} from '..';

const options: RouterOptions = {
strict: false,
caseSensitive: false,
mergeParams: false
};

const r = new Router();
const router = new Router(options);
const routerHandler: RouteHandler = (req, res) => {
res.end('FIN');
};

router.get('/', routerHandler);
router.post('/', routerHandler);
router.delete('/', routerHandler);
router.patch('/', routerHandler);
router.options('/', routerHandler);
router.head('/', routerHandler);
router.unsubscribe('/', routerHandler);

// param
router.param('user_id', (req, res, next, id) => {
const val: string = id;
next();
});

// middleware
router.use((req, res, next) => {
next();
});

const api: IRoute = router.route('/api/');

router.route('/')
.all((req, res, next) => {
const url: string = req.baseUrl;
next();
})
.get((req, res) => {
res.setHeader('x-header', 'value');
res.end('.');
});


// test router handling
var server = createServer(function(req: IncomingMessage, res: ServerResponse) {
router(req, res, (err) => {
if (err) {
const e: Error = err;
}
//
})
router.handle(req, res, (err) => {
//
})
})
22 changes: 22 additions & 0 deletions tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
{
"compilerOptions": {
"module": "commonjs",
"lib": [
"es6"
],
"esModuleInterop": true,
"noImplicitAny": false,
"noImplicitThis": true,
"strictNullChecks": true,
"strictFunctionTypes": true,
"types": ["node"],
"noEmit": true,
"forceConsistentCasingInFileNames": true
},
"include": [
"test/**/*"
],
"files": [
"index.d.ts"
]
}