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

Added ZipCompressionStream & ZipDecompressionStream #488

Merged
merged 2 commits into from
Feb 24, 2024
Merged

Added ZipCompressionStream & ZipDecompressionStream #488

merged 2 commits into from
Feb 24, 2024

Conversation

BlackAsLight
Copy link
Contributor

Related to this issue #487 adds new two classes ZipCompressionStream and ZipDecompressionStream to work with the .pipeThrough and .pipeTo methods available on ReadableStreams.

I have not tested the changes I made to the repo as I'm not sure how to exactly with there seeming to be rollup in the mix and I also haven't used nodejs in ages. Let me know if you want me to make any other changes to it though.

@gildas-lormeau
Copy link
Owner

gildas-lormeau commented Feb 22, 2024

Thank you very much for your efforts. It looks fine to me, except for 3 details at first glance:

  • would it be possible to name the classes ZipReaderStream and ZipWriterStream? I'd find that more coherent, as I don't use the terms "Compress" and "Decompress" in the API.
  • today zip.js doesn't use the JS private members (e.g. #field or #method()) even though the code is partly based on ES2020. For the sake of consistency, it would be preferable not to use this feature for the time being. I'd prefer to refactor the code globally in zip.js to introduce this technical feature.
  • you have to import these new classes into all the files that import zip-reader.js and zip-writer.js. An alternative would be to define the 2 new classes directly in these files.

If you wish, I can merge your PR and take care of these minor changes. It's up to you. In both cases, you will appear as a contributor ;)

@BlackAsLight
Copy link
Contributor Author

Made those changes. You can feel free to merge and make any more changes yourself or let me know if you'd like me to make any more changes myself.

@BlackAsLight
Copy link
Contributor Author

I also just noticed that my IDE did some white space changes that I didn't notice earlier. I can revert those if you'd like

@gildas-lormeau gildas-lormeau merged commit 4149806 into gildas-lormeau:master Feb 24, 2024
@gildas-lormeau
Copy link
Owner

Thank you very much for your work. I've just published the new version with your contributions :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants