Replace jsunzip with modern libraries (fflate + pako) #506
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Remove custom TINF gzip implementation in favor of maintained library - Part of broader effort to replace jsunzip with standard libraries - Maintains existing ungzip() API and error handling - All existing gzip tests continue to pass 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove custom JSUnzip implementation in favor of maintained library - Use fflate's unzipSync for synchronous ZIP handling - Add comprehensive ZIP tests with sample files - Maintains existing unzipDiscImage() and unzipRomImage() APIs - All existing functionality continues to work - Fix lint-staged to only run prettier on supported file types 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Delete custom jsunzip implementation (628 lines) - Update lib/README to remove jsunzip reference - All tests continue to pass with new fflate/pako implementation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Owner
Author
|
Should lay a tiny bit of groundwork for #505 (though not directly in this form) |
- Optimize Uint8Array usage to avoid unnecessary buffer copying - Update test-zip.js to use vitest expect API for consistency - Fix path handling to use path.join for cross-platform compatibility 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the custom jsunzip implementation with maintained libraries and updates related tests and docs.
- Swap out custom ZIP/GZIP code for
fflate.unzipSyncandpako.ungzipinsrc/utils.js - Remove the legacy
src/lib/jsunzip.jsfile and references - Add comprehensive ZIP tests covering SSD/ROM, mixed archives, and error cases
Reviewed Changes
Copilot reviewed 5 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils.js | Replaced jsunzip logic with fflate and pako calls |
| src/lib/jsunzip.js | Deleted legacy JSUnzip library |
| tests/unit/test-zip.js | Added new ZIP unit tests for various archive scenarios |
| src/lib/README | Removed jsunzip reference |
| package.json | Added fflate and pako dependencies, updated formatting rules |
Comments suppressed due to low confidence (1)
tests/unit/test-zip.js:49
- [nitpick] The comment says this ZIP is incompatible but the test loads
test-ssd.zip, which actually contains a valid SSD file; please update the comment to match the test data.
// Create a simple ZIP with incompatible file
|
|
||
| function unzipImage(data, knownExtensions) { | ||
| const unzip = new jsunzip.JSUnzip(); | ||
| console.log("Attempting to unzip"); |
There was a problem hiding this comment.
[nitpick] Consider removing or replacing this debug console.log with a proper logger or debug-level logging to avoid polluting production output.
| const match = filename.match(/.*\.([a-z]+)/i); | ||
| if (!match || !knownExtensions[match[1].toLowerCase()]) { | ||
| console.log("Skipping file", f); | ||
| console.log("Skipping file", filename); |
There was a problem hiding this comment.
[nitpick] This debug console.log could be removed or switched to a debug logger to keep production logs clean.
| } | ||
| if (uncompressed) { | ||
| console.log("Ignoring", f, "as already found a file"); | ||
| console.log("Ignoring", filename, "as already found a file"); |
There was a problem hiding this comment.
[nitpick] Consider removing or demoting this console log to a debug channel to prevent excessive log output in production.
| throw new Error("Failed to uncompress file '" + loadedFile + "' - " + uncompressed.error); | ||
| } | ||
|
|
||
| console.log("Unzipped '" + loadedFile + "'"); |
There was a problem hiding this comment.
[nitpick] This final console.log is useful for debugging but might be noisy in a production environment; consider using a configurable logger.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
unzipSyncAPI)src/lib/jsunzip.jsfile completelyunzipDiscImage(),unzipRomImage(),ungzip()Technical Decisions
Testing
Files Changed
src/utils.js: Updated compression/decompression functionspackage.json: Added fflate and pako dependenciessrc/lib/jsunzip.js: Removed (628 lines deleted)src/lib/README: Updated to remove jsunzip referencetests/unit/test-zip.js: New comprehensive ZIP teststests/unit/zip/directory🤖 Generated with Claude Code