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

Add TypeScript Support #6

Open
alex-barber opened this issue Mar 7, 2021 · 7 comments
Open

Add TypeScript Support #6

alex-barber opened this issue Mar 7, 2021 · 7 comments

Comments

@alex-barber
Copy link

It would be great to use this in TypeScript projects. I don't know much about publishing typescript packages on NPM, but I'd happily try and contribute towards that effort.

@maetl
Copy link
Owner

maetl commented Mar 7, 2021

Thanks Alex, that is a good idea. I will look into what’s involved. I think adding a types key in package.json pointing to a .d.ts header file would be a good starting point, that may not necessitate porting the entire codebase to TypeScript in one go.

May also be worth reconsidering the file structure and build scripts to output a more clean, 2021 compatible dependency structure for this lib. I’m no longer compiling my recent work to CommonJS and would prefer to distribute ES6 compatible imports by default for everything—but have not gone back and amended Rung to support this way of doing things.

@maetl
Copy link
Owner

maetl commented Mar 25, 2022

@alex-barber FYI I’ve completed the ESM compatible build changes and begun work on converting this lib to TypeScript. I can potentially publish a new version now with a types/typingskey pointing to signatures for all the main functions—however it doesn’t look like too much additional effort to move the source files over as well. Let me know if you want to help out.

@alex-barber
Copy link
Author

@maetl I would be very excited to help! Thanks much for remembering my comment, I appreciate it. Let me know what you think some good first steps could be.

@maetl
Copy link
Owner

maetl commented Mar 29, 2022

@alex-barber thanks for responding. I think a good place to start would be testing the TypeScript definition for the main package when npm install rung is run in a fresh project and making sure that it works at the code level, but also that IDE completions etc make sense.

@maetl
Copy link
Owner

maetl commented Mar 29, 2022

There is some additional work needed to remove a bunch of stray any types that got generated by the typescript compiler. The simplest approach for now is probably adding jsdoc comments with @param and @return types marked correctly.

After that, I’d like to take a look at the definition of algorithms and define these functions as a type that takes a seed : number and returns a callable next() : number type. This possibly needs to be a union with Math.random if that is possible, or some equivalent fn() : number, as that is compatible with existing usages that use it directly (eg: const rng = new Random(Math.random)).

Haven’t thought much further than this. I will go through and add docs bit by bit over the next few days when I have time.

@alex-barber
Copy link
Author

alex-barber commented Apr 26, 2022

Hello, sorry for the late reply! I have been using rung in a few TS projects this past month and it has worked like a charm, it's really been a huge help. Thanks so much for making it ✌️

The only thing I've noticed is that the documentation is slightly inaccurate, rng.integer(10) listed as the first example in the README needs two arguments to function, and there is no example using rng.decimal, which I've used a lot since finding it in the source code.

As far as IDE completions go, I was initially confused by the output when calling seed with no argument. With an argument I get a consistent value, and without I get random. After looking up the stuff going on with Date.now it makes a lot of sense, and it's very convenient it works that way. I think it might be a detail worth including in the README.

@maetl
Copy link
Owner

maetl commented May 3, 2022

Thanks for the excellent feedback on improvements to the docs. Missing rng.decimal may have been an oversight. And I agree that the README could have a better explanation for seeding.

I will look into the rng.integer args situation. It is another one of those “Count from zero or count from one? Inclusive by default or exclusive?” situations where there isn’t an obvious intuitive answer (at least for me). I think I have gone back and forward on this API on different projects. It is in need of clarification and support for overloading.

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

No branches or pull requests

2 participants