kriasoft / universal-router

A simple middleware-style router for isomorphic JavaScript web apps
https://www.kriasoft.com/universal-router/
MIT License
1.7k stars 104 forks source link

Add TypeScript typings #159

Closed frenzzy closed 5 years ago

frenzzy commented 5 years ago

Types of changes

Checklist:

Closes #101

@pradyuman, @dangmai, @anantoghosh, @gytisgreitai, @jtmthf, @tlaziuk, @Lodin would be cool if some of you guys could take a look, thanks in advance!

codecov-io commented 5 years ago

Codecov Report

Merging #159 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #159   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         144    144           
  Branches       42     42           
=====================================
  Hits          144    144

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6319db3...54e8b40. Read the comment docs.

gytisgreitai commented 5 years ago

Well I'm a bit out of Typescript world, so some of those maybe rookie mistakes.

I've replaced version in package.json with "git+https://github.com/frenzzy/universal-router.git#54e8b4047b987a1e7d75f7e3a1588c02ea31e2a0" and here are some observations:

  1. VSCode compiles fine, shows bindings all ok
  2. yarn start when using react-scripts-ts fails with Module not found: Can't resolve 'universal-router'
  3. How do I do imports with default class export ? E.g. now I'm doing, is there a prettier way?:
    import UniversalRouter from 'universal-router';
    import { Route, Context } from 'universal-router';
  4. In Route the action param signature is as follows:
    action?: (context: RouteContext<C, R> & C, params: Params) => R | Promise<R> | void

shouldn't context be optional? cause I can simply do action: () => (<Dashboard/>) and it does seem to work

frenzzy commented 5 years ago

Hey @gytisgreitai, thanks for your feedback!

  1. Cool! This is the main goal why I want to add typings to the package.
  2. The package requires compilation before usage, it is why you can't just use git url. Instead you have to do the following:
    git clone https://github.com/frenzzy/universal-router.git
    cd universal-router
    git checkout typescript
    yarn install
    yarn build
    cd dist
    yarn link
    cd ../../your-project-name
    yarn link universal-router
    yarn start
  3. Like this:
    import UniversalRouter, { Route, Context } from 'universal-router';
  4. From TypeScript docs:

Optional Parameters in Callbacks

Don’t use optional parameters in callbacks unless you really mean it:

/* WRONG */
interface Fetcher {
    getObject(done: (data: any, elapsedTime?: number) => void): void;
}

This has a very specific meaning: the done callback might be invoked with 1 argument or might be invoked with 2 arguments. The author probably intended to say that the callback might not care about the elapsedTime parameter, but there’s no need to make the parameter optional to accomplish this – it’s always legal to provide a callback that accepts fewer arguments.

Do write callback parameters as non-optional:

/* OK */
interface Fetcher {
    getObject(done: (data: any, elapsedTime: number) => void): void;
}