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

refactor: rewrite to typescript retaining API #320

Merged
merged 6 commits into from
Sep 10, 2024
Merged

refactor: rewrite to typescript retaining API #320

merged 6 commits into from
Sep 10, 2024

Conversation

MarshallOfSound
Copy link
Member

@MarshallOfSound MarshallOfSound commented Aug 16, 2024 •
edited
Loading

Modernize the codebase substantially. Key things:

  • Git claims I rewrote these files from scratch, I did not, the actual changes are just adding types and in a few edges cases adding additional error handling (that typescript revealed as possible)
  • I did not touch the tests at all, I had to fix like 5 imports, and prettier added a bunch of semicolons, but the actual tests were not "fixed" or "changed" at all. They will be converted to typescript after this PR lands to ensure confidence

Closes #251

@MarshallOfSound MarshallOfSound requested a review from a team as a code owner August 16, 2024 07:56
@MarshallOfSound MarshallOfSound force-pushed the typescript branch 6 times, most recently from 15f21d7 to 5d1304c Compare August 16, 2024 08:07
@MarshallOfSound MarshallOfSound mentioned this pull request Aug 16, 2024
Open
erikian
erikian reviewed Aug 16, 2024
View reviewed changes
Copy link
Member

@erikian erikian left a comment

Choose a reason for hiding this comment

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

just one issue regarding yarn.lock, otherwise looks good

src/asar.ts Outdated Show resolved Hide resolved
src/asar.ts Outdated Show resolved Hide resolved
src/asar.ts Show resolved Hide resolved
src/filesystem.ts Show resolved Hide resolved
src/wrapped-fs.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
dsanders11
dsanders11 reviewed Sep 7, 2024
View reviewed changes
Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

Not blockers, but a comment about TSDoc and some suggestions for improving type coverage.

src/asar.ts Outdated Show resolved Hide resolved
src/asar.ts Outdated Show resolved Hide resolved
src/asar.ts Outdated Show resolved Hide resolved
src/disk.ts Outdated Show resolved Hide resolved
src/filesystem.ts Outdated Show resolved Hide resolved
src/integrity.ts Outdated Show resolved Hide resolved
@MarshallOfSound MarshallOfSound merged commit 88b5ea4 into main Sep 10, 2024
3 checks passed
@MarshallOfSound MarshallOfSound deleted the typescript branch September 10, 2024 01:21
@continuous-auth Continuous Auth
Copy link

🎉 This PR is included in version 3.2.11 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsanders11 dsanders11 dsanders11 approved these changes

@erikian erikian erikian approved these changes

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

Successfully merging this pull request may close these issues.

Typescript rewrite?
3 participants
@MarshallOfSound @dsanders11 @erikian