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.

Sign up for GitHub

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

Jump to bottom

feat: Port library to TypeScript #37

Merged
merged 3 commits into from Oct 28, 2020
Merged

feat: Port library to TypeScript #37

merged 3 commits into from Oct 28, 2020

Conversation

fb55
Copy link
Owner

@fb55 fb55 commented Oct 20, 2020

  • Also introduces ESlint, Prettier & Jest.
  • Adds missing tests for full coverage.
  • Adds JSDoc comments to all methods.

@fb55
feat: Port library to TypeScript
0d6dc03 
* Also introduces ESlint, Prettier & Jest.
* Adds missing tests for full coverage.
* Adds JSDoc comments to all methods.
@fb55 fb55 requested a review from feross October 20, 2020 22:27
@fb55
Copy link
Owner Author

fb55 commented Oct 23, 2020

@feross Forgot to leave a comment — I know that Webtorrent is the main user of this module. How do you feel about this change? Will it cause any issues for you?

@feross
Copy link
Collaborator

feross commented Oct 27, 2020

A couple questions:

  • Does this change the public API?
  • Were the tests changed or just ported to typescript verbatim? It would have been nice to leave the old tests in place and update them in a future PR, since if you rewrite the package and the tests in the same PR, you can't test if the new rewrite still passes the old tests.

feross
feross reviewed Oct 27, 2020
View reviewed changes
Copy link
Collaborator

@feross feross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay – lgtm! It seems the only public API change is removing the grow property. So this should be published a new major version.

src/index.ts
* Grow the bitfield up to this number of entries.
* @default 0.
*/
private readonly grow: number;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this private should be fine

@fb55
Copy link
Owner Author

fb55 commented Oct 28, 2020

Thanks a lot for the review!

It would have been nice to leave the old tests in place and update them in a future PR, since if you rewrite the package and the tests in the same PR, you can't test if the new rewrite still passes the old tests.

That's a great point, thanks for raising it. I'll keep it in mind going forward.

@fb55 fb55 merged commit bf3af48 into master Oct 28, 2020
@fb55 fb55 deleted the feat/ts branch October 28, 2020 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feross feross feross approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@fb55 @feross