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

Typescript declaration file #531

Closed
wants to merge 2 commits into from

Conversation

jimmythecoder
Copy link

I wrote a Typescript typings declaration file for using AutoNumeric as an ambient JS include in a TypeScript project. It contains most methods and properties, but not all. Some of the documentation isn't clear but it will be fine for 90% of projects.

First draft, not yet complete. Need to copy from documentation all methods and configuration options.
Most* methods and properties are now declared, but there may still be a few remaining. Some methods have complex overloading which have not yet been defined. Should be fine for most projects.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 62.472% when pulling b90df06 on jimmythecoder:master into e42d0a1 on autoNumeric:master.

@AlexandreBonneau AlexandreBonneau self-assigned this Nov 20, 2017
Copy link
Member

@AlexandreBonneau AlexandreBonneau left a comment

Choose a reason for hiding this comment

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

Does Typescript users really need that file?
Can't the compiler/IDE find out the types using the JSDoc comments from the code?

I'm asking because having to maintain that manually seems like a waste of time when it looks like an automated mean could be used?

Thank you for the effort though :>

new (selector: string | HTMLInputElement, options?: Options): Input;
new (selector: string | HTMLInputElement, defaultValue: number, options?: Options): Input;

multiple(selector: string | HTMLInputElement[], options?: Options): Input;
Copy link
Member

Choose a reason for hiding this comment

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

The multiple() function does not only accept an array of HTMLInputElement as its first argument, but non-input DOM elements too (HTMLElement).

It can also in place of an array, accept an object like { rootElement: HTMLElement } or { rootElement: HTMLElement, exclude: Array<HTMLInputElement>}.

See the JSDoc!

declare module AutoNumeric {

interface Static {
new (selector: string | HTMLInputElement, options?: Options): Input;
Copy link
Member

Choose a reason for hiding this comment

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

The constructor also accepts non-input DOM elements too (HTMLElement).

version(): string;
}

interface Input {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to rename that to something else, since AutoNumeric can also work with non-input element?
This could be confusing.

Shouldn't that be named AutoNumeric since it the object class name?
Perhaps I'm missing something here though :)

@jimmythecoder
Copy link
Author

Hi! Thanks for the review, Typescript does not use or understand JSdoc other than intellisense hints. An ambient .d.ts is always required to consume a 3rd party lib.

I feel that typescript is the most useful thing in front end since jQuery so I highly recommend supporting it :) I’m happy to go through your comments and make changes, AutoNumeric is a large lib so I havnt defined everything in there yet and as your comments pointed out some things are amiss. Some JavaScript statements are also highly dynamic and take some thinking in order to type correctly I may have simplified it a bit to get my own code working such as multiple constructors or multiple return types with generics takes a deep understand of the code itself not always visible from the docs so your welcome to correct anything in there :)

I’m sure as frameworks grow and become more widely used especially angular these ambient ts files are super helpful and provide the best programmers guidance on how to use the api, very nice in vscode with intellisense for method names params and returns.

Cheers!

@AlexandreBonneau
Copy link
Member

Have you ever tried automated tools like tsd-jsdoc or jsdoc-to-typescript-declaration before?
What are you thoughts on those?
Couldn't they be used to automate the tasks of creating such file?

I'm open to add the definition file if it can improve the dev life, but if we were to have such file, then it needs to be always up-to-date with the codebase since we strive to deliver only top quality code (we had an old d.ts file before, be we scrapped it since it was largely outdated, and nobody complained about it!).

That means that having to do it by hand is a no-go since it adds even more steps to do when publishing a new version (ie. changing the source code header, package.json, changelog, readme.md, documentation component on the autonumeric.org, etc.), that's why I'm asking about an automated way to generate it.

I recognize your efforts and the usefulness of that file, the problem however as I see it is maintaining it.

@chadbengen
Copy link

FWIW i created a more comprehensive definition file here .

@AlexandreBonneau
Copy link
Member

Thanks @chadbengen !
Is there any way to merge both your great works together @jimmythecoder?

@AlexandreBonneau
Copy link
Member

Hey @jimmythecoder, @chadbengen, is a merge something you would be willing to do?

@chadbengen
Copy link

@AlexandreBonneau I am not able to put in any time to do that personally. If @jimmythecoder wants to merge the files he has my blessing. If i make any changes or updates i'll be sure to post something.

@AlexandreBonneau AlexandreBonneau added the 💁‍♂️ Need reporter feedback A response is needed from the reporter before being able to do more label Sep 5, 2018
@mseele
Copy link
Contributor

mseele commented Jan 15, 2020

Any news on this?

@AlexandreBonneau
Copy link
Member

@mseele #661 is on the way, which merge work from this PR

@AlexandreBonneau AlexandreBonneau removed the 💁‍♂️ Need reporter feedback A response is needed from the reporter before being able to do more label Jan 16, 2020
@AlexandreBonneau
Copy link
Member

The updated definition file has been merged in #661, based on your work.
Thanks @jimmythecoder and @chadbengen !

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