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

Feature/typescript support #52

Closed
wants to merge 2 commits into from

Conversation

vahdet
Copy link

@vahdet vahdet commented Aug 8, 2020

This PR introduces Typescript to the content of src directory in such ways:

  • Added tsconfig.json and convert file extension from .js to .ts, then solve the typing problems. Introduced shared and isolated types/interfaces (Kept them detailed as possible, but left as any where I cannot figure out).

  • Changed eslint configuration for Typescript .

  • Removed Babel dependencies and .babelrc file, as the current structure do not need it (But, it may be tested more specifically).

  • Converted the npm build command to tsc (See in package.json).

  • Oh, finally, put the language notations code sections in markdown files to make them look better on (at least) the local IDEs.

@vahdet vahdet mentioned this pull request Aug 8, 2020
@JamesBrill
Copy link
Owner

Many thanks for making these changes, @vahdet - I certainly learned something from reading them.

I must admit I wasn't expecting the library itself to be rewritten in TypeScript, just the addition of a declaration file. Is there any advantage to consumers of this library for it to be written in TypeScript? Or is a declaration file sufficient? We now have types defined under @types here.

I do not wish to undermine your efforts here, but if the declaration files in DefinitelyTyped are sufficient to enable simple integration of this library into TypeScript projects, then I would prefer it to remain vanilla JavaScript, mainly because I prefer to keep the build simple for development and the language one that I'm familiar with. That's not to say I don't see the benefits of TypeScript - I may choose to adopt it in future, in which case your PR here will serve as a fantastic starting point for making that migration.

@vahdet
Copy link
Author

vahdet commented Aug 17, 2020

@JamesBrill You are right, this PR goes well beyond the adding TypeScript support as the types are added to the DefinitelyTyped by @OleksandrYehorov as stated here. All in all, this lib can now be used by TypeScript projects -which was the main motive.

And, yes, it can be a reference once you consider rather a TypeScript re-write. Thus, I do not think postponing/closing this PR is a loss of effort or such (I had already did the changes on my fork before deciding to send as a PR). So, feel free :)

@JamesBrill
Copy link
Owner

I'll close this PR and the issue as TypeScript consumers seem to be unblocked for now. Thanks for raising the issue and providing these changes for reference.

@JamesBrill JamesBrill closed this Aug 17, 2020
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

3 participants