pillarjs / router

Simple middleware-style router
MIT License
410 stars 102 forks source link

Typescript Typings #48

Open origin1tech opened 8 years ago

origin1tech commented 8 years ago

Any chance of getting some @types/router typings published? Or are there any floating around that we can make official and publish.

blakeembrey commented 8 years ago

@types has no relation to this module, if you want someone in the community to type it you can ask at https://github.com/DefinitelyTyped/DefinitelyTyped which is where @types is published from. You can also contribute them yourself. I'll leave this issue open to add a TypeScript definition to the router itself, but it sounds like you're after something else that doesn't exist (yet).

origin1tech commented 8 years ago

actually no I'm talking about the type definitions. However it would be appropriate to publish once done to @types for ease of install with Typescript 2.x. Sorry if I wasn't clear, but yes we'd need the defs first obviously :)

dougwilson commented 8 years ago

@origin1tech I'm not sure if third parties even have the rights to publish under other user's namespaces, like the @types namespace. I think @blakeembrey has the right path forward, unless you can share some documentation on how we would be able to publish under that npm namespace?

I personally have never written typings before, so probably someone who knew how to write them would have to (I think @blakeembrey does).

blakeembrey commented 8 years ago

@origin1tech I do know what you're talking about. DefinitelyTyped is the location you should ask if you want something published to @types. The @types on NPM has zero relation to the module itself. I would hopefully know, I am a member of @DefinitelyTyped, wrote @types and @typings and helped TypeScript with the @types implementation 😄

blakeembrey commented 8 years ago

@dougwilson We can't, and we wouldn't want to. Once someone has written a definition, we can add it to the module ourselves and publish to NPM directly (using typings in package.json like https://github.com/pillarjs/path-to-regexp/blob/master/package.json#L6). This makes it easier for TypeScript users as they can forgo the installation from @types altogether and it just works.

origin1tech commented 8 years ago

@blakeembrey I'm fully aware of that....my point is simply that it would be nice to get defs for router and then publish an @types project for ease of install. I get that they are unrelated, however if the author creates the definitions it's helpful as there's less chance of things getting missed.

blakeembrey commented 8 years ago

@origin1tech You're missing the point. Please re-read my comments. If the goal is "ease of install" we would add them to this module, and not @types. That way, as a TypeScript user, you don't have to do anything to have it work - which is ease of install to me, compared to having to install an additional module. That's why I suggested keeping it open for that, and to ignore @types.

origin1tech commented 8 years ago

Maybe I'm missing something but Typescript is suggesting @types as the path for installing defs. I talk to some of those fellas on the regular pretty certain on that.

see https://blogs.msdn.microsoft.com/typescript/2016/06/15/the-future-of-declaration-files/

dougwilson commented 8 years ago

@origin1tech that article has the following line:

Specifically, Blake Embrey, the maintainer and creator of Typings, has worked closely with us during this entire process and given us valuable feedback

And you are talking to Blake in this very thread :)

blakeembrey commented 8 years ago

@origin1tech You're incorrect. That approach is relevant only for third-party typings (E.g. those from Typings/DefinitelyTyped). Modules themselves can publish their own type definitions, and that has been the preferred approach for first-party typings since TypeScript 1.5. Since you opened the issue on the module itself, I would assume the best approach is the easiest for you as a user.

origin1tech commented 8 years ago

@blakeembrey how does that work if you exclude "node_modules" in your tsconfig.json. How does it get imported unless you specify it in "typeRoots" manually?

blakeembrey commented 8 years ago

@origin1tech You can read up on how exclude works in the TypeScript handbook, but it won't break that. exclude is there to ignore "entry-points" of the project. If something requires into node_modules or elsewhere, it'll still load those type definitions as required for type checking. It wouldn't be a very effective process if that didn't work 😄 If you want to test it out, give it a go on the module I linked above.

echo "import pathToRegexp = require('path-to-regexp')" > index.ts
npm install path-to-regexp
tsc index.ts

The process of looking up native typings is related to moduleResolution in TypeScript, so it needs to be using moduleResolution: node.

blakeembrey commented 8 years ago

Here's a response by Daniel on the same article you posted.

image

blakeembrey commented 8 years ago

If you've used Typings before, @types is basically doing the same resolution, filling that gap in tooling - just all natively within the NPM and TypeScript ecosystems now instead of having separate tools for it. You can also see in Typings that it has been the advice for authors since release (https://github.com/typings/registry#contributing).

origin1tech commented 8 years ago

@blakeembrey seen that before but was under the impression any external module, best practice would be @types.

testing it on some of own projects really quick...

blakeembrey commented 8 years ago

But why? Imagine you have a native TypeScript project. It wouldn't make any sense to make the author's life more complicated. Just like here, if the author's are willing to have a .d.ts file, why not allow them to own it? Otherwise it ends up with relying on the community to make the needed changes, which just an extra barrier to entry. For example, in the past we've (Typings) merged native type definitions into dozens of JavaScript-native projects including moment and es6-promise (and path-to-regexp, I suppose!). Most of the time it just takes a PR 😄

origin1tech commented 8 years ago

@blakeembrey No I'm agreeing on that one...makes me wonder why bother with a separate registry at all. seems like best practice would be for someone to create defs then make a PR. I can't imagine many authors would be too opposed to that.

blakeembrey commented 8 years ago

There are some. It's a maintenance hassle for authors that don't care about TypeScript support natively. Also, sometimes the author just doesn't have someone good at TypeScript. For Typings, I tried to alleviate this by offering support everywhere (see https://github.com/typings/typings/issues/322) but it's still tricky because GitHub doesn't have org mentions. For example, recently I wanted to use Facebook's dataloader project and I was like, wow, someone already made the PR. But, unfortunately, that person didn't quite understand TypeScript themselves and the definition that has been published is incorrect. See my PR in https://github.com/facebook/dataloader/pull/51. This sort of thing happens pretty regularly when people start learning TypeScript but haven't learnt how to write or test a definition (E.g. how the different export types relate to JavaScript).

Edit: Not to conflate those two issues, but having the third-party location does make sense. It can also be helpful for "beta"/incomplete typings. For example, not all typings are finished right away - many can require many iterations to fix. Having to make many PRs to something else can increase friction. I can only imagine what'd it be like for me developing https://github.com/types/npm-rethinkdb but under @rethinkdb without proper access myself. There's times where I make a dozen changes because it's a huge definition.

origin1tech commented 8 years ago

Right...yeah run into that a fair amount. Getting better but early on. Well in big projects it's worth it in our opinion. We you have so many working on a proj it saves a lot of headache just in refactoring alone. But ur right lots of odd defs with things exported multiple times missing overloads on methods lots of stuff see it all at this point.

...see quite a bit of this with Ionic not too long ago just as an example. TS is a bit of an afterthought I think for them

franciscosucre commented 5 years ago

Any update on this issue? Thanks!

blakeembrey commented 5 years ago

@franciscosucre You're certainly welcome to submit an initial PR and we can go from there 😄

franciscosucre commented 5 years ago

Hello @blakeembrey i am no typing expert. I would not know how to start this typings.

For anybody still looking for a solution, this allowed me to continue to work with typescript using the router. Microsoft has a type definition generator, it is not perfect but it allowed to me to continue to work.

npm install -g dts-gen
dts-gen.cmd -m router

I do not know if the generated typings could work out for the devs as a starting point @blakeembrey .

franciscosucre commented 5 years ago

¿How would i upload this type definitions? It is a single file and i would not know where to use place them in this proyect ¿ Can i upload them here in this issue or something?

ilikejames commented 5 years ago

I've been looking into this, and I'm keen to get typings for this package. This is the most popular package I've ever seen without any typings!

The issue I see currently is:

index.js#L76

function Router(options) {

  function router(req, res, next) {
    router.handle(req, res, next)
  }

  // inherit from the correct prototype
  setPrototypeOf(router, this)
  return router
}

Router.prototype = function () {}

The docs then say to execute route matching using:

README.md#L235


var router = Router()
var server = http.createServer(function onRequest(req, res) {
  router(req, res, finalhandler(req, res))
})

This overriding the prototype constructor doesn't appear possible with typescript definitions (although if someone knows I'd love to know how). It also seems weird and deviates from express-serve-static-core implementation.

If people are willing to drop this returned method and make a breaking change to a new version. This would be dropping the constructor prototype override and instead moving to the handle method. I can look into hand rolling the types and see if theres any more issues.

dougwilson commented 5 years ago

It also seems weird and deviates from express-serve-static-core implementation.

I'm not sure what this is, can you elaborate?

If people are willing to drop this returned method and make a breaking change to a new version.

I'm not really interested in making such a breaking change if the only purpose is to support typescript. This module is written in javascript to be javascript and use the features that javascript makes available. If the desire was to restrict the code to only what is possible in typescript, the code would just have been written in typescript. Someone can always make a typescript friendly fork I assume, no?

wesleytodd commented 5 years ago

Someone can always make a typescript friendly fork I assume, no?

I am not sure why this would be necessary. Is the issues that TS typing cannot understand inheriting from a function? If so, then seems like an issue on their end. If I understand the issue correctly, could it not just pretend it is an object? I know it would ignore it's 'function'-ness in the documentation/autocomplete, but is that really an issue?

dougwilson commented 5 years ago

One additional thought I had to add to the above: is it actually impossible to represent in typescript definitions, or are you just saying you're not sure how to represent it in the definitions?

ilikejames commented 5 years ago

Apologies. I think it is possible. I've hacked together some of the api using the express-serve-static-core typings.


declare module 'router' {

 interface RequestHandler {
    (req: Request, res: Response, next: NextFunction): any;
  }
  interface IRouter {
    get: IRouterMatcher<this>;
    post: IRouterMatcher<this>;
    put: IRouterMatcher<this>;
    delete: IRouterMatcher<this>;
    patch: IRouterMatcher<this>;
    options: IRouterMatcher<this>;
    head: IRouterMatcher<this>;

    use: (path: PathParams, router: IRouter) => void;

    handle: RequestHandler
  }

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

  var Router: RouterConstructor;

  export default Router;
}

This would be without a breaking change.

Edit: This gives the following:

const router = new Router()
router.get('/whatever', (request: Request, response: Response) => undefined); 
router(request, response, finalErrorHandler); 

TODO: handle const router = Router() but I'm thinking its do-able. I'll continue working on it over the coming days but I see there is a lot of mutation of the request/response objects and I'm not sure how far you deviate from the express router in this regard.

tangye1234 commented 3 years ago
// router.d.ts
// Type definitions for router

declare module 'router' {
  import { NextFunction, NextHandleFunction } from 'connect'
  import { IncomingMessage, ServerResponse } from 'http'

  export type Path = string | RegExp | Array<string | RegExp>

  export namespace Router {
    export interface RouteType {
      new (path: string): Route
      prototype: Route
    }

    type Method = 'all' | 'head' | 'get' | 'post' | 'delete' | 'put' | 'patch' | 'options'

    export type Route = { readonly path: Path } & Record<Method, (middleware: NextHandleFunction, ...middlewares: NextHandleFunction[]) => Route>

    export interface Options {
      caseSensitive?: boolean
      strict?: boolean
      mergeParams?: <C extends {}, P extends {}>(currentParams: C, parentParams: P) => Record<string | number, any>
    }

    export type ParamCallback<K = string | number> = (
      req: IncomingMessage,
      res: ServerResponse,
      next: NextFunction,
      value: any,
      name: K,
    ) => any

    interface InnerRouter extends NextHandleFunction {
      route(path: Path): Route
      param: <K extends string | number>(name: K, fn: ParamCallback<K>) => this
    }

    export type Router = InnerRouter & Record<'use' | Method, {
      (path: Path, middleware: NextHandleFunction, ...middlewares: NextHandleFunction[]): Router
      (middleware: NextHandleFunction, ...middlewares: NextHandleFunction[]): Router
    }>

    interface RouterType {
      new (options?: Options): Router
      (options?: Options): Router
      Route: RouteType
      prototype: Router
    }
  }

  export type RouterType = Router.RouterType
  const Router: RouterType
  export default Router
}
t-pk commented 1 year ago

any update on this? :(((