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

[Not Done] Add JSDoc typing #6

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DrJKL
Copy link

@DrJKL DrJKL commented Dec 9, 2024

Addresses #1

Not done. I'll wait to see if @shanewholloway is okay with this direction.

Questions

  • Is seed always expected to be a string?
  • What should state be?
  • Should xg just be {next(): number;}?
  • What all opts are there? (I can just go through the code and find uses when I have some free time this week)

@shanewholloway
Copy link
Owner

shanewholloway commented Dec 10, 2024

From a short review, this approach takes a dependency on TypeScript (in package.json), transforming the project into a TypeScript based-project. I'd prefer not to do this, if practical. How does the .d.ts sidecar information get bundled under Rollup into the published esm/ directory?

The Typed JavaScript JSDoc approach seems workable, if verbose, and sidesteps a dependency on TypeScript. It may also adds some direct value for those browsing the source code directly in node_modules/ or GitHub.

Regarding your listed questions, if I once knew, that cache has long been cleared! 😅
My intent, as I remember, was for this project to be as close to a direct port to ESM as feasible, and to maintain 1:1 reproduction of each function's outputs given the same inputs. The test suite can act as guide to some of the parameters. I don't remember if the test suite used every API interface afforded by the original seedrandom module. That will likely require an audit/review of the code, as you propose in the last question bullet.

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.

2 participants