Convert project to TypeScript #146
Conversation
adroitwhiz
force-pushed
the
typescript
branch
from
3df675b
to
27792a2
Compare
|
@PullJosh @towerofnix This PR is ready for review now! |
There was a problem hiding this comment.
I left a shorter review covering just the files through src/Project.ts. This is looking awesome! Just a few comments. I'll get to the remaining files later, just taking a break for the morning now and figure you can use what I've got so far. 📦
Another thing I wanted to note is that you've got some very specific style things going on here, e.g. parentheses around single-argument arrow functions, and commas following the last item in array/object literals. Are these being formatted or verified automatically? I didn't see any "scripts" in package.json doing so but might have missed it.
I think this PR would be an OK time to implement an auto-format script with prettier (explicitly configured how you're using it — sb-edit doesn't follow these rules I commented on). But feel free to disagree with me on that and save it for a following PR!
| public name: string; | ||
| public url: string; | ||
| public img: HTMLImageElement; | ||
| public isBitmap: boolean; |
There was a problem hiding this comment.
Not an issue for this PR but let's revisit this isBitmap flag when dejankifying/reworking costume loading.
There was a problem hiding this comment.
This is really awesome work! I've left a bunch of comments throughout, with a number of questions, since I'm still relatively new to TypeScript, and some notes to file issues for new TODO's. Most of my remarks are tidbits/small-scale — on the whole this pull request is great, and really improves the legibility and reliability of Leopard code. Thank you!
adroitwhiz
force-pushed
the
typescript
branch
2 times, most recently
from
11a8222
to
b76a34a
Compare
There was a problem hiding this comment.
Tested this out locally with...
- Colorlens by yuski347 to confirm overall behavior correctness (complex project which is 98% functioning - both before and after this PR!)
- Audio effects demo by me to confirm audio effects are still working (yep!)
I've requested one change but otherwise this is LGTM! 🎉
I think it'd be a good idea to start putting together a "project corpus" as a list of projects, preferably some complex ones included, demonstrating various behaviors which are currently functioning in Leopard, which we can test new larger pull requests like this one against.
src/Sprite.ts
Outdated
|
|
||
| protected _vars: Vars; | ||
|
|
||
| public constructor(initialConditions: InitialConditions, vars: Vars) { |
There was a problem hiding this comment.
We discussed the vars option previously; to follow up, removing the default value for vars (or a nullish coalesce) is a breaking behavior change, because generated sprite code expects the variables object to exist.
I've branched into leopard-js/sb-edit#109 for discussion; if we implemented that, it would technically be fine to drop the default {} here, because Scratch 3.0 doesn't support dynamically creating variables (AFAIK toLeopard.ts neither, though 2.0 does!), so all variables will always have an initial value and thus vars will be provided. But I think it's still best to just guarantee the shape of SpriteBase.vars here.
There was a problem hiding this comment.
See comment on behavior change + We should probably still do this._vars = vars ?? {} in src/Sprites.ts even as we're implementing leopard-js/sb-edit#109, which is a separate pull request + projects may still depend on this.vars existing without providing initial variable values.
|
I've removed the |
There was a problem hiding this comment.
LGTM!
Sorry about the Var/object shenanigans! Let's keep working out the right way forward for that in leopard-js/sb-edit#109. I think we're better off merging this into master so we have a clean commit base to work on further TODO's from.
We just made the OK to push recent changes to sb-edit and Leopard online — I think it would make sense to push that release without this PR included, just because I don't know if we've done a ton of behavior testing on the changes here yet. Most changes here are hardly semantic at all, but I'd like to move to more comprehensive project testing anyway.
My idea would be to merge this, and throughout the time before next release, pull up a bunch of Scratch projects, manually testing projects and making a list of which ones are partially or entirely broken. Then, narrow down and find which behaviors Leopard currently isn't compatible with that we might've missed, and file issues for those, trying to pick off what we can. (Thread sequencing - plus support for stop other scripts in sprite - is probably a big one rn, for example.)
Compatibility should be better now than it used to be, but I haven't done any extensive testing to really get a feel for where we're at, and I'd like to focus on that myself for a while (as well as continuing reviews for others' pull requests).
It'll also help spot any compatibility issues which might've snuck through in this code rework - I doubt there are many, if even any, but I want to be thorough before pushing large-scale changes to the main site. How's all that sound?
Thanks again for all your work on this pull request, and for your input on issues outside this PR, too!
|
Want to bump Node to 18 in this pull request, since it already covers a pretty wide range of structural shenanigans? ( |
|
Sure! I've just bumped the Node version in the workflow. |
|
Rebased against #173 now that that's merged! |
|
How can I best help with this? Is more review or testing needed? |
|
@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! |
|
@PullJosh I'm leaning towards merging this PR sometime this week to unblock future improvements. Do you want to look over it first? |
|
@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. 🙂 |
|
👍 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. |
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.