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

Rewrite in Typescript 🔥 #166

Closed
wants to merge 7 commits into from

Conversation

langpavel
Copy link
Collaborator

@langpavel langpavel commented Apr 9, 2019

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

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, ⚙️/ts-lib-starter

TODO:

  • check if all the outputs are compatible
  • building in same place
  • building in similar way
  • add possibility for custom route type as template type
  • backport types (.d.ts) to 8.x version
  • move relevant test to typescript, run beforer build + write consistency check tests (after build)

I need help with review, testing and feedback!

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

@codecov-io
Copy link

codecov-io commented Apr 9, 2019

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
Copy link
Member

frenzzy commented Apr 9, 2019

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
Copy link

piglovesyou commented Apr 10, 2019

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
Copy link
Collaborator Author

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
Copy link
Member

frenzzy commented Apr 10, 2019

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
Copy link
Collaborator Author

langpavel commented Apr 10, 2019

@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. ⌛

@langpavel langpavel changed the title Rewrite in Typescript :hot: Rewrite in Typescript 🔥 Apr 19, 2019
@langpavel
Copy link
Collaborator Author

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
Copy link
Member

frenzzy commented May 1, 2019

I created a PR with improved typings based on yours: #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
Copy link
Member

frenzzy commented Nov 6, 2019

I am going to rethink this PR after pillarjs/path-to-regexp#203 get released, JFYI.

@frenzzy frenzzy mentioned this pull request Jan 2, 2020
12 tasks
@frenzzy
Copy link
Member

frenzzy commented Jan 2, 2020

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
Copy link

@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.

@frenzzy frenzzy closed this in #183 Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants