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

Update TypeScript and enable strict mode #94

Merged
merged 21 commits into from
Dec 16, 2023

Conversation

adroitwhiz
Copy link
Collaborator

@adroitwhiz adroitwhiz commented Feb 28, 2023

Resolves #92
Resolves #97

This PR updates TypeScript to the latest release (4.9), updates Prettier to the next major version, and enables strict mode.

Some externally-visible changes have been made to the codebase:

  • Sound.sampleCount and Sound.sampleRate are now nullable
  • Numeric fields are now always serialized to .sb3 as strings
  • When deserializing, null block inputs are skipped completely for now
    • I'll revisit this later. If there's, for instance, an empty C-block inside a .sb3 file, its SUBSTACK will be either null or completely absent in the .sb3's block inputs, depending on whether a stack was ever dragged into the C-block. We previously used to treat the two cases differently, treating the input as {type: "string", value: null} in the former case (flat-out wrong), and leaving the input out in the latter case. Now, we always leave the input out. It'll be easier to fix this properly once we have a better way to enumerate a block's intended inputs-- see Proposal: New API + set of types for working with blocks in a type-safe way #93.
  • Null procedure_call arguments are now {type: 'boolean', value: false}, which matches what they are when exported from Scratch.

I haven't touched the Leopard serialization code yet, since #90 touches it a lot. I'll go over it and un-draft this PR once it's merged.

@adroitwhiz adroitwhiz marked this pull request as ready for review March 8, 2023 03:30
@adroitwhiz adroitwhiz force-pushed the strict branch 3 times, most recently from 3e0ba36 to 4e90e5a Compare March 9, 2023 18:24
@adroitwhiz
Copy link
Collaborator Author

adroitwhiz commented Mar 9, 2023

PS: While any (existing) code style issues can be noted, I don't think they should be blockers here. I see this PR as a prerequisite to other changes (including fixing those issues), and fixing #100 will mean re-coding a substantial portion of this project regardless, which will likely bulldoze over any code style issues.

Copy link
Member

@towerofnix towerofnix left a comment

Choose a reason for hiding this comment

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

Thanks, this looks awesome! I appreciate how much more guarded strict mode is over null/undefined, and I'm assuming it also played a role in the rest of the improvements here, which look great. 🚀

I have just a few remarks and one or two minor requested changes — this PR is pretty much good-to-go as is. Thanks for all your work on it!

.github/workflows/nodejs.yml Show resolved Hide resolved
src/Block.ts Show resolved Hide resolved
src/BlockInput.ts Show resolved Hide resolved
src/Data.ts Show resolved Hide resolved
src/io/leopard/toLeopard.ts Show resolved Hide resolved
src/io/sb3/fromSb3.ts Outdated Show resolved Hide resolved
src/io/sb3/fromSb3.ts Show resolved Hide resolved
src/io/sb3/interfaces.ts Show resolved Hide resolved
src/io/sb3/interfaces.ts Show resolved Hide resolved
src/io/scratchblocks/toScratchblocks.ts Outdated Show resolved Hide resolved
Copy link
Member

@towerofnix towerofnix left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!

@towerofnix
Copy link
Member

Since we've gone through a thorough review of this earlier, I'm calling this good to merge.

There are loads of force pushes above on my part - sorry about that! These were to fix unaddressed lint and snapshot issues in-place as they cropped up, rather than later commits. The history should be good here now, it just took a couple tries to get right. ^^;

@towerofnix towerofnix merged commit 7bb34ba into leopard-js:master Dec 16, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants