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

Registry entry for typed-jszip #620

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sebastianhaas
Copy link
Member

Typings URL: https://github.com/sebastianhaas/typed-jszip

Questions (for new typings):

  • Does the README explain the purpose of the typings and have a link to the JavaScript project?
  • Do the typings follow the source structure (e.g. index.js <-> index.d.ts)?
  • Are they external or global modules according the source (e.g. see README sources)?
  • Are the tests running

I took most of the definitions from DefinitelyTyped, as stated in the readme.

There are still two problems existing, I don't really know how to resolve them so I would be glad if somebody could help out.

_Definitions issue_
JSZip uses a constructor-like function to create an instance of JSZip. Since they both have the same name, but the first one is an interface in a namespace, using it in typescript will currently result in a rather ugly

  let zip: JSZip.JSZip = new JSZip();

Is there a way to make JSZip both the name of an interface as well as an object holding a constructor like function?
https://stuk.github.io/jszip/documentation/api_jszip/constructor.html

_Browser test issue_
I wrote tests for all major use-cases, server-side runs fine, but I don't really have experience with jspm and I can't get it running on the browser-side. It appears that generator-typings is not producing ready-to-use framework for the browser atm.

@sebastianhaas sebastianhaas changed the title Added reference to sebastianhaas/typed-jszip. Registry entrz for typed-jszip Jul 12, 2016
@sebastianhaas sebastianhaas changed the title Registry entrz for typed-jszip Registry entry for typed-jszip Jul 12, 2016
@blakeembrey
Copy link
Member

For your issue, I'd recommend you just do a refactor of the definition into a single class and re-export it. It would make for nicer intellisense also. For instance:

// Make the main export a class.
declare class JSZip {
  constructor ();
  constructor (data: any, options?: JSZip.Options);
}

// Use the namespace to store interfaces and such.
declare namespace {
  export interface Options {
    ...
  }
}

// Export `JSZip` `module.exports =` style.
export = JSZip

@blakeembrey
Copy link
Member

Oh, actually - I see you might also want to support the plain function (non-class) style of constructor. I can't remember where I've done that style before, but you can't use the class syntax for it. I'll think on it, I feel I've done that someplace before and should be possible - I might submit a PR later if I try it out.

@felixfbecker
Copy link
Contributor

@blakeembrey class syntax supports calling without new? Not afaik. If you find it out please hit me up because I'm using the static/instance interface pattern everywhere and it bloats definition files.

@blakeembrey
Copy link
Member

blakeembrey commented Aug 27, 2016

@felixfbecker You can't, I mentioned it above (#620 (comment)) - I'd just overlooked that usage. The preferred way to do it would be an inline value/type. Something like:

interface JSZip {
  something: boolean
}

var JSZip: {
  (): JSZip
  new (): JSZip
}

Edit: Or following the TypeScript precedent would be creating ObjectConstructor interfaces, for example.

@felixfbecker
Copy link
Contributor

@blakeembrey I misread your comment, I thought you said you can use the class syntax for it 😅

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

Successfully merging this pull request may close these issues.

3 participants