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

Type definitions for JSZip #601

Merged
merged 2 commits into from Apr 19, 2020
Merged

Type definitions for JSZip #601

merged 2 commits into from Apr 19, 2020

Conversation

deliangyang
Copy link

Suport typescript Type definitions

@deliangyang
Copy link
Author

Sorry to ping @Stuk could this PR be reviewed?

Copy link
Owner

@Stuk Stuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pulling this together, it seems really comprehensive! I'm excited to get TS definitions in this project.

I'm still ramping up on some of the more advanced usage of Typescript types, so have left some questions for my own clarification.

compressedContent: string|ArrayBuffer|Uint8Array|Buffer;
}

type InputFileFormat = InputByType[keyof InputByType];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InputFileFormat's type is the element of interface InputByType, like string, an array of number, Unit8Array, ArrayBuffer or Blob.

dosPermissions: number | null;
options: JSZipObjectOptions;

_data: CompressedObject;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is private, so I don't think it should be part of the TS interface.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but sometimes we need to get the compression size and origin file's size.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I'm not comfortable explicitly exposing deliberately private properties.

Could you remove this property from this PR, and open another PR that exposes this property explicitly?

* @param options Optional information about the file
* @return JSZip object
*/
file<T extends JSZip.InputType>(path: string, data: InputByType[T] | Promise<InputByType[T]>, options?: JSZip.JSZipFileOptions): this;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On use does one need to write file<Blob>(...), or will it be inferred from the data argument?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll be inferred from the data argument.

* @param onUpdate The optional function called on each internal update with the metadata.
* @return A Node.js `ReadableStream`
*/
generateNodeStream(options?: JSZip.JSZipGeneratorOptions<'nodebuffer'>, onUpdate?: OnUpdateCallback): NodeJS.ReadableStream;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, so the type is a string (nodebuffer) instead of the type itself?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a string and not the type?

@Regnander
Copy link

Any updates on this?

@re2005
Copy link

re2005 commented Apr 17, 2020

Hi guys, thanks for the awesome JSZip.

I'm using TS and wonder if you guys have intention to merge this PR ?
Would be great.

Thanks

@Stuk Stuk merged commit 20ad7aa into Stuk:master Apr 19, 2020
@sschmidTU
Copy link

sschmidTU commented Apr 21, 2020

We're using TypeScript and JSZip 3.4.0 failed our CI build. Needed just a little configuration change, but still a breaking change.

before (breaking with 3.4.0):

import JSZip = require("jszip");
// starting with jszip 3.4.0, JSZip.JSZip is not found,
//    probably because of new possibly conflicting TypeScript definitions
const zip: JSZip.JSZip = new JSZip();
// (zip.load...)
return zip.file("test.xml").async("string");

now working:

import JSZip from "jszip";
const zip: JSZip = new JSZip();
// (zip.load...)
return zip.file("test.xml").async("text");

This may also have to do with us using
"esModuleInterop": true,
in tsconfig.json since recently, but still, jszip 3.3.0 was still passing builds.

I also was a bit surprised that "string" was suddenly not valid anymore, the typescript definition allows various strings now, "text" one of them.

also, using the package es6-promise caused a conflict with Promise, but it was apparently unnecessary anyways, so i simply removed it.

Our greenkeeper CI issue, for reference (references our fixing commit):
opensheetmusicdisplay/opensheetmusicdisplay#723

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.

None yet

5 participants