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

Allow decompress option for non-deflate compressed entries #81

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

overlookmotel
Copy link
Contributor

@overlookmotel overlookmotel commented Apr 24, 2018

PR for #80.

Currently it is not possible to extract the raw compressed data for entries compressed with a method other than 8 (Deflate).

If you try to open a read stream for an entry compressed with e.g. Deflate64:

  • with option decompress: false, you get error "options.decompress can only be specified for compressed entries"
  • with no decompress option, you get error "unsupported compression method: 9"

This PR alters the treatment of the decompress option in .openReadStream() so you can pass option decompress: false and get a stream of the compressed data.

Ideally, I think it'd be better to implement this by altering entry.isCompressed() to use entry.compressionMethod !== 0. But isCompressed() is part of the public API, so that would be a semver-major change. This PR as it stands would (I think) not.

@thejoshwolfe
Copy link
Owner

Thanks for writing up this PR. I'm inspired to take this seriously, and I've started working on this in a branch (wip). Getting the code working is pretty trivial, as you've demonstrated, but I want to be careful about how this changes the openReadStream API.

I'm not happy with how complicated things are getting in the docs and the code wrt what cases are allowed and not. I'm considering some kind of api simplification that just asks for the raw data and ignores the compression method and encryption scheme and all that. 🤔 I'll continue working on this tomorrow or sometime soon.

@thejoshwolfe
Copy link
Owner

See #82

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