Improve ZIP compatibility#27
Conversation
|
@ctalkington had chance to take a look? |
| ZipArchiveOutputStream.prototype._appendStream = function(ae, source, callback) { | ||
| ae.getGeneralPurposeBit().useDataDescriptor(true); | ||
| ae.setVersionNeededToExtract(constants.MIN_VERSION_DATA_DESCRIPTOR); | ||
| if (ae.getMethod() === constants.METHOD_DEFLATED) { |
There was a problem hiding this comment.
i feel like this is going to cause issues. because with streaming we dont have the size yet. so the code in _writeLocalFileHeader (https://github.com/archiverjs/node-compress-commons/blob/master/lib/archivers/zip/zip-archive-output-stream.js#L405) is going to write zeros for size. since normally when a data descriptor is used the size is fixed at the end.
to keep the streaming interface and use this logic, we would have to buffer all the content leading to higher memory usage.
There was a problem hiding this comment.
I was following similar logic here. But I've since updated the PR. Although, I'm not sure I follow? This seems to work fine?
| this.name = null; | ||
| this.size = -1; | ||
| this.csize = -1; | ||
| this.size = 0; |
There was a problem hiding this comment.
i think was left over from older code and i thought it got set to 0 from one of the consumer modules but this will ensure such is a sane default. thanks.
| util.getShortBytes = function(v) { | ||
| var buf = new Buffer(2); | ||
| buf.writeUInt16LE(v, 0); | ||
| buf.writeUInt16LE((v & 0xFFFF) >>> 0, 0); |
There was a problem hiding this comment.
seems like a good idea. thanks.
|
@ctalkington Updated PR |
|
thanks for following up. working on getting a batch of bugfixes out this weekend. |
METHOD_STOREDis used (e.g:java.util.zip.ZipInputStream)writeUInt32LEandwriteUInt16LEto preventout of boundsexceptionNOTE: this should fix compatibility for Java Archives