feat(zip): cd header and zip64 info generation implemented #2792
Conversation
This file contains 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
ibgreen
approved these changes
Nov 18, 2023
| @@ -0,0 +1,96 @@ | |||
| export const signature = new Uint8Array([0x01, 0x00]); | |||
|
|
|||
| /** info that can be placed into zip64 field */ | |||
There was a problem hiding this comment.
Add a link to some specification in the comment?
| * @param options info that can be placed into zip64 field | ||
| * @returns buffer with field | ||
| */ | ||
| export const createZip64Info = (options: Zip64Options): ArrayBuffer => { |
There was a problem hiding this comment.
Nit: Prefer function syntax for longer functions. IIMHO, the => arrow notation is best for callbacks and one liners.,
Suggested change
| export const createZip64Info = (options: Zip64Options): ArrayBuffer => { | |
| export function createZip64Info(options: Zip64Options): ArrayBuffer { |
|
|
||
| let result = new ArrayBuffer(0); | ||
|
|
||
| zip64Fields.forEach((field) => { |
There was a problem hiding this comment.
Nit: Prefer for loops:
Suggested change
| zip64Fields.forEach((field) => { | |
| for (field of zip64Fields) { |
| zip64Length: (options.offset ? 1 : 0) * 8 + (options.size ? 1 : 0) * 16 | ||
| }; | ||
|
|
||
| let result = new ArrayBuffer(0); |
There was a problem hiding this comment.
Repeated concatenation is expensive (you end up copying the same data over and over again). Better to save an array of chunks and concatenate at the end.
Suggested change
| let result = new ArrayBuffer(0); | |
| const chunks: ArrayBuffer[] = []; | |
| ... | |
| return concatenateArrayBuffers(chunks); |
| } | ||
| const newValue = new DataView(new ArrayBuffer(field.size)); | ||
| setNumbers[field.size](newValue, 0, optionsToUse[field.name ?? ''] ?? field.default); | ||
| result = concatArrays(result, newValue.buffer); |
There was a problem hiding this comment.
Agan, avoid repeated concatenation...
|
|
||
| const encodedName = new TextEncoder().encode(optionsToUse.fileName); | ||
|
|
||
| const resHeader = concatArrays(concatArrays(header.buffer, encodedName), zip64header); |
There was a problem hiding this comment.
Import from loaders.gl/loader-utils, see comment below.
| } | ||
| const header = new DataView(new ArrayBuffer(46)); | ||
|
|
||
| fields.forEach((field) => |
There was a problem hiding this comment.
Use for loop, see comment below
Comment on lines
+229
to
+232
| if (Object.keys(optionsToZip64).length) { | ||
| zip64header = createZip64Info(optionsToZip64); | ||
| optionsToUse.extraLength = zip64header.byteLength; | ||
| } |
There was a problem hiding this comment.
Perhaps we can have this function return null if no keys, rather than check length of keys here?
Suggested change
| if (Object.keys(optionsToZip64).length) { | |
| zip64header = createZip64Info(optionsToZip64); | |
| optionsToUse.extraLength = zip64header.byteLength; | |
| } | |
| const zip64header = createZip64Info(optionsToZip64); | |
| if (zip64Header) { | |
| optionsToUse.extraLength = zip64header.byteLength; | |
| } |
| * @param options info that can be placed into cd header | ||
| * @returns buffer with header | ||
| */ | ||
| export const generateCdHeader = (options: CDGenOptions): ArrayBuffer => { |
There was a problem hiding this comment.
- Use
functionkeywors - I recommend preserving capitalization rather than decapitalizing to fit the camelCase.
- Also be consistent with function names and option types
Suggested change
| export const generateCdHeader = (options: CDGenOptions): ArrayBuffer => { | |
| export function generateCDHeader(options: GenerateCDOptions): ArrayBuffer { |
| @@ -188,3 +189,167 @@ const findExpectedData = (zip64data: Zip64Data): {length: number; name: string}[ | |||
|
|
|||
| return zip64dataList; | |||
| }; | |||
|
|
|||
| /** info that can be placed into cd header */ | |||
| type CDGenOptions = { | |||
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.
Part of changes I'll need in order to create slpk with hash file inside