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

Added typescript definition files #76

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
language: node_js
node_js:
- "0.10"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add these back; this version of the module supports these Node.js versions. Removing support is a major version bump. You can retarget your PR to the 2.0 branch if you want to make a breaking change, though.

Copy link
Author

@ilikejames ilikejames Jan 8, 2019

Choose a reason for hiding this comment

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

I'll retarget as typescript won't work with such old versions of node. I'll first try seeing if there is allow_failures that can be used for this specific test.

I'll also thinking that this maybe better pushed to @DefinitelyTyped. This way there would be locked @typings/router@1.3.3 version aligned with a publish router@1.3.3 and leave it to the community to update. I often use older package versions simply due to the @type support.

Looking down the list of PRs for 2.0 I not sure what methods we could do to keep these aligned, except by being rigorous for any forthcoming changes.

Up to you guys really. Let me know.

- "0.12"
- "1.8"
- "2.5"
Expand Down Expand Up @@ -28,5 +27,6 @@ script:
# Run test script, then lint depending on eslint install
- "npm run-script test-travis"
- "test -z $(npm -ps ls eslint) || npm run-script lint"
- "npm run-script test-types"
after_script:
- "test -d .nyc_output && npm install coveralls@2 && nyc report --reporter=text-lcov | coveralls"
138 changes: 138 additions & 0 deletions index.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
export = Router;

declare namespace Router {

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

interface IncommingRequest {
url: string,
method: string,
originalUrl?: string,
params?: any;
}

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

type RequestParamHandler = (req: IncommingRequest, res: any, next: NextFunction, value: any, name: string) => any;

interface RouteHandler {
// tslint:disable-next-line callable-types (This is extended from and can't extend from a type alias in ts<2.2
(req: RoutedRequest, res: any, next: NextFunction): any;
}

interface RequestHandler {
// tslint:disable-next-line callable-types (This is extended from and can't extend from a type alias in ts<2.2
(req: IncommingRequest, res: any, next: NextFunction): any;
}


interface NextFunction {
// tslint:disable-next-line callable-types (In ts2.1 it thinks the type alias has no call signatures)
(err?: any): void;
}

type ErrorRequestHandler = (err: any, req: IncommingRequest, res: any, next: NextFunction) => 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 samesignature 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;

/**
* Alternatively, you can pass only a callback, in which case you have the opportunity to alter the app.param()
*
* @deprecated since version 4.11
*/
param(callback: (name: string, matcher: RegExp) => RequestParamHandler): this;

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

get: IRouterMatcher<this>;
post: IRouterMatcher<this>;
put: IRouterMatcher<this>;
delete: IRouterMatcher<this>;
patch: IRouterMatcher<this>;
options: IRouterMatcher<this>;
head: 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.

There seems to be a lot more methods missing here. Can you add the rest? Examples are .bind, .propfind, etc.

Copy link
Author

Choose a reason for hiding this comment

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

.bind and all the default object level props you get for free. I can't find .propfind in the codebase. Not on 2.0 either?

Copy link
Contributor

Choose a reason for hiding this comment

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

All these methods are not in the code base -- they are dynamic depending on the Node.js version -- even the ones like .options is not in the code base at all...

We iterate over https://nodejs.org/dist/latest-v10.x/docs/api/http.html#http_http_methods and add the lower case as a method to the router prototype. Here is propfind existing:

$ node -pe 'require("router")().propfind'
[Function]

Copy link
Author

@ilikejames ilikejames Jan 8, 2019

Choose a reason for hiding this comment

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

Ah. So, on my node version i have:

[
     'ACL',
     'BIND',
     'CHECKOUT',
     'CONNECT',
     'COPY',
     'DELETE',
     'GET',
     'HEAD',
     'LINK',
     'LOCK',
     'M-SEARCH',
     'MERGE',
     'MKACTIVITY',
     'MKCALENDAR',
     'MKCOL',
     'MOVE',
     'NOTIFY',
     'OPTIONS',
     'PATCH',
     'POST',
     'PROPFIND',
     'PROPPATCH',
     'PURGE',
     'PUT',
     'REBIND',
     'REPORT',
     'SEARCH',
     'SUBSCRIBE',
     'TRACE',
     'UNBIND',
     'UNLINK',
     'UNLOCK',
     'UNSUBSCRIBE' 
],

So all these should be present I guess.

First thoughts are that this would be correct, but quite noisy, burying more useful day-to-day commands. I've took the original list from the express definitions.
They would be accessible still via array notation route['subscribe']().

I'm guessing this would also be different per node version?
I'm going to be using this library both via node and within browser and this list is bigger than the browser support list.

Its not going to be possible to a run-time version, so I'm wondering what the best direction is now. Full list, partial list of most common, or just documenting that the full set of HttpMethods are available?

Edit: So I see the require('methods'). Obviously this makes this unsuitable for me to use isomorphically from within a web browser, which is annoying. But, as for this PR what do you suggest? As this list of methods is created at runtime it won't be possible to do that same for the types.

My thought it should be a documentation "feature", with the major 99.99% of use cases covered via type definitions, with the documentation saying the full list of methods are available depending on node version via router['method']();.

I'm unsure. What do others think?

Copy link
Member

Choose a reason for hiding this comment

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

So I see the require('methods'). Obviously this makes this unsuitable for me to use isomorphically from within a web browser

Not true. This package is the foundation of the routing I do with nighthawk and works perfectly fine on the browser side. I believe all of the bundlers have browser pollyfills for the methods, but even if not the methods package has a basic list it uses.

Copy link
Member

Choose a reason for hiding this comment

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

For example, this is the shim that browserify uses: https://github.com/jhiesey/stream-http/blob/master/index.js#L58-L85

My thought it should be a documentation "feature", with the major 99.99% of use cases covered via type definitions

I think this is a fine approach as long as the popular tooling can handle undocumented usage well.


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

handle: RequestHandler;

route(prefix: PathParams): IRoute;
/**
* Stack of configured routes
*/
stack: any[];
}

interface IRoute {
path: string;
stack: any;
all: IRouterHandler<this>;
get: IRouterHandler<this>;
post: IRouterHandler<this>;
put: IRouterHandler<this>;
delete: IRouterHandler<this>;
patch: IRouterHandler<this>;
options: IRouterHandler<this>;
head: IRouterHandler<this>;
}

interface RouterConstructor extends IRouter {
new(options?: RouterOptions): IRouter & RequestHandler;
}

}

declare var Router: Router.RouterConstructor;
11 changes: 8 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,28 +18,33 @@
"utils-merge": "1.0.1"
},
"devDependencies": {
"@types/node": "^10.0.0",
"after": "0.8.2",
"eslint": "3.19.0",
"eslint-plugin-markdown": "1.0.0-beta.6",
"finalhandler": "1.1.1",
"mocha": "3.5.3",
"nyc": "10.3.2",
"supertest": "1.2.0"
"supertest": "1.2.0",
"typescript": "^2.9.2"
},
"files": [
"lib/",
"LICENSE",
"HISTORY.md",
"README.md",
"index.js"
"index.js",
"index.d.ts"
],
"types": "index.d.ts",
"engines": {
"node": ">= 0.10"
},
"scripts": {
"lint": "eslint --plugin markdown --ext js,md .",
"test": "mocha --reporter spec --bail --check-leaks test/",
"test-cov": "nyc --reporter=text npm test",
"test-travis": "nyc --reporter=html --reporter=text npm test"
"test-travis": "nyc --reporter=html --reporter=text npm test",
"test-types": "tsc --project tsconfig.json --noEmit"
}
}
61 changes: 61 additions & 0 deletions test/type-check.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import { createServer, IncomingMessage, ServerResponse } from 'http';
import {
default as Router,
RouterOptions,
IncommingRequest,
RouteHandler,
IRoute,
NextFunction,
RoutedRequest
} from '..';


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

const r = new Router();
const router = new Router(options);
const routerHandler: RouteHandler = (req: IncommingRequest, res: any, next: NextFunction) => {};

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

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

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

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

router.route('/')
.all((req: RoutedRequest, res: any, next: NextFunction) => {
next();
})
.get((req, res) => {
// do nothing
});


// test router handling

var server = createServer(function(req: IncomingMessage, res: ServerResponse) {
router(req as IncommingRequest, res, (err: any) => {
//
})
router.handle(req as Router.IncommingRequest, res, (err: any) => {
//
})
})



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"
]
}