search

kriasoft / universal-router

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

Rewrite in Typescript 🔥 #166

Closed langpavel closed 4 years ago

langpavel commented 5 years ago

Types of changes

Checklist:

This is complete rewrite in typescript

May still need some edits, but I think that we can create new branch and pre-release (I already changed version in package to 9.0.0-rc1) Build is working, tests too, but I used base from mine starter, :gear:/ts-lib-starter

TODO:

I need help with review, testing and feedback!

/cc @frenzzy @koistya ref #101 @pradyuman @dangmai @gytisgreitai ref #159

codecov-io commented 5 years ago

Codecov Report

Merging #166 into master will decrease coverage by 26.05%. The diff coverage is 72.88%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #166       +/-   ##
===========================================
- Coverage     100%   73.94%   -26.06%     
===========================================
  Files           7        3        -4     
  Lines         181      357      +176     
  Branches       53      104       +51     
===========================================
+ Hits          181      264       +83     
- Misses          0       67       +67     
- Partials        0       26       +26
Impacted Files Coverage Δ
dist/universal-router-generate-urls.js 44.93% <44.93%> (ø)
dist/universal-router-sync.js 96.96% <96.73%> (ø)
dist/universal-router.js 97% <96.77%> (ø)

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 7cd21a1...6745a0f. Read the comment docs.

frenzzy commented 5 years ago

Hi! Thank you for the PR. It looks like a lot of work has been done, impressive!

Could you please explain your motivation regarding rewriting the library to TypeScript?

So far I tend to decline this changes by the following reasons:

  1. I do not see profit for users, we already provide builtin type definitions.
  2. It makes it harder to understand the source code for users which are not familiar with TS.
  3. TS compiler usually generates not size optimized code.
piglovesyou commented 5 years ago

Personally I always welcome this kind of change 👍 TS is basically more maintainable than JS. Because that's why TS exists users will indirectly get benefits though continuous package maintenance. Also I feel general trend where more packages of @types/* will be rewritten in TS now.

langpavel commented 5 years ago

Hi @frenzzy Reason is that type definitions are not complete and it is more paint to use universal-router with typescript now so I think short time about moving away. Instead I take one day to make this change. it is easier to rewrite all in typescript then trying to fix type definitions. Now I'm asking for help from community to test it and suggest enhancements. And of course, if someone can copy generated definitions and merge it in master, this will be great. Unfortunately I have no more time to work on this (week or two), help will be really welcomed!

It makes it harder to understand the source code for users which are not familiar with TS.

I'm not sure. In fact i rewrite this into typescript to understand how code works! :-) And people reading code will appreciate type annotations as semantics metadata, not having debugger in head running on 100% :-)

frenzzy commented 5 years ago

Correct me if I am wrong but to add type annotations it is not required to rewrite the codebase to TS, for example you can reach the same effect with JSDoc only.

Reason is that type definitions are not complete

Let's just complete them? It should be even simpler than a complete rewriting. Maybe you can generate definitions from your TS version and we will compare what we missed?

it is more paint to use universal-router with typescript

But why, what the difference?

langpavel commented 5 years ago

@frenzzy

Let's just complete them? It should be even simpler than a complete rewriting.

Yeah, but adding annotations may be dangerous because they may become obsolete and useless and this is reason why I rewrite it in TS.

Adding types to existing source and let typescript really validate code is safe way how to create type definitions. I did this only to get valid

Maybe you can generate definitions from your TS version and we will compare what we missed?

Yes, this was original purpose, but when I did this, I created this PR, IMHO adopting typescript is best way to continue with project

And sorry, I'm really out of time for next 14 days. :hourglass:

langpavel commented 5 years ago

I’m gonna be busy for the next two weeks. Please, test, review and may be publish as experimental prerelease. It's working. There may be only little divergences which can be easily narrowed I believe.

frenzzy commented 5 years ago

I created a PR with improved typings based on yours: https://github.com/kriasoft/universal-router/pull/167

it is more paint to use universal-router with typescript

Hopefully the source of the problem is eliminated and we no longer need this PR.

frenzzy commented 4 years ago

I am going to rethink this PR after https://github.com/pillarjs/path-to-regexp/pull/203 get released, JFYI.

frenzzy commented 4 years ago

I have tried to continue this work in #183 but I see problems which worrying me:

  1. Usage of <R, C> too many times looks wrong, but I have no idea if it is solvable.
  2. It is not clear for me how properly to test typings and make sure they are usable.
piglovesyou commented 4 years ago

@frenzzy

  1. I think it's all right unless users pass the same generic params multiple times. Passing generic types always looks like it.
  2. Writing tests with stricter options such as noImplicitAny: true would be a good idea to prove if the types work correctly.