Skip to content
This repository has been archived by the owner on Apr 16, 2022. It is now read-only.

Typescript? #70

Closed
akdor1154 opened this issue Nov 6, 2017 · 5 comments
Closed

Typescript? #70

akdor1154 opened this issue Nov 6, 2017 · 5 comments
Assignees
Milestone

Comments

@akdor1154
Copy link
Contributor

akdor1154 commented Nov 6, 2017

Hey,
I'm trying to write a PR to add map support and I'm finding that there are a lot of extraneous checks in this project for whether properties exist or not, etc. It's always good to be careful with js, but I think the level you are going to here is leading to a quite difficult to read codebase.

Would you accept a PR to write this in Typescript instead of es6? I'm happy to do the work, you're already transpiling (it would replace Babel), and I think it would lead to much cleaner code (the typescript compiler is smart enough to know when certain checks are not required, so you can largely rely on build-time safety instead of the hand-written runtime checks you are using now).

I know this would be quite an invasive change, so that's why I'm checking instead of barging ahead and PR/forking. If you're unsure maybe we could chat over some IM medium?

Cheers
Jarrad

@martysweet
Copy link
Owner

Hi Jarrad,

I think implementing TypeScript is a good call, 100% agree on the readability issues.

Thanks,
Marty

@akdor1154
Copy link
Contributor Author

Awes. I have a little time on this now so will try and get it done in the next day or two. I'll start with a verbatim translation to get it through then work on tightening it up after (much much easier to refactor with a compiler to help you). Kind of related - what's your branching strategy? Should I be PR-ing against master or release? If release, then what is the purpose of master?

@akdor1154
Copy link
Contributor Author

aaand one more thing, can I target Node 4? or do you want to keep support for Node 0.x?

@martysweet
Copy link
Owner

@akdor1154 PRs should be against the development branch. I will update the CONTRIBUTING.md and README.md files.

I would be happy with min Node 4, looks like v0.12.x was EOL at the end of 2016. Will also update the documents regarding this.

Thanks for all your work on this, much appreciated!

martysweet pushed a commit that referenced this issue Nov 13, 2017
Addresses issue #70 
* typescript infrastructure
* es6 -> ts
* Convert to Typescript
* Move tests into src
@martysweet martysweet mentioned this issue Nov 13, 2017
5 tasks
@martysweet martysweet added this to the v1.3.0 milestone Nov 19, 2017
@martysweet
Copy link
Owner

Released in v1.3.4

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants