search

leopard-js / leopard

Library for making Scratch-like projects with JavaScript.
https://leopardjs.com
MIT License
138 stars 26 forks source link

Convert project to TypeScript #146

Closed adroitwhiz closed 1 year ago

adroitwhiz commented 2 years ago

Resolves #131

There's a lot left to do here--for instance, class visibility modifiers and making the API signatures explicit--but I'm opening this as a draft PR for feedback + any contributions.

EDIT: Let's punt on figuring out method visibility and API surface for now.

adroitwhiz commented 1 year ago

@PullJosh @towerofnix This PR is ready for review now!

adroitwhiz commented 1 year ago

I've removed the Vars type parameter for now in order to allow a default parameter value for variables (the two can't be provided at the same time). I've changed clone-related methods to return whatever subclass of Sprite they're called on, so that if a user overrides _vars on their Sprite subclass, they can access them on the clones.

towerofnix commented 1 year ago

Want to bump Node to 18 in this pull request, since it already covers a pretty wide range of structural shenanigans? (.github/workflows/npm-publish.yml, https://github.com/leopard-js/sb-edit/pull/94#discussion_r1132672194)

adroitwhiz commented 1 year ago

Sure! I've just bumped the Node version in the workflow.

towerofnix commented 1 year ago

Rebased against #173 now that that's merged!

HanClinto commented 1 year ago

How can I best help with this? Is more review or testing needed?

towerofnix commented 1 year ago

@HanClinto This is good to go, it's just pending on a review from @PullJosh. Thanks for the offer to help! If you'd like to offer an extra set of eyes, you're welcome to go over changes and leave comments/questions - if you're interested in working on Leopard's internals, it's a good opportunity to familiarize yourself with the details and code patterns introduced here!

adroitwhiz commented 1 year ago

@PullJosh I'm leaning towards merging this PR sometime this week to unblock future improvements. Do you want to look over it first?

PullJosh commented 1 year ago

@adroitwhiz Honestly, with school and life going on, I don't think I'll have time. But if you and @towerofnix both feel good, I am totally on board with merging. 👍 It's definitely a good policy to require review and approval from other contributors before merging, but I don't feel that there's any reason in particular that review needs to go through me. 🙂

towerofnix commented 1 year ago

👍 We dug through this PR pretty thoroughly and have been working off its code for a while too, so I'm quite confident saying it's LGTM.