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

Add TypeScript description of API #22

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ntninja
Copy link

@ntninja ntninja commented Feb 9, 2018

Hi! I've been using your library for a while and I've recently started to use the TypeScript type checker for my project and noticed that your library has no description and quickly wrote one that gets the job done (both UMD and browser should work, but I only tested the later).

If you're interested in this please make a new release that includes this file and push it to NPM so that the introspection and typing features offered by TypeScript will start to just work for people using that. If not, then feel free to just turn down this PR. I won't be offended or anything. 🙂

@lupomontero
Copy link
Owner

Hi @Alexander255,

Many thanks for the PR 👍

I'm not (yet) familiar with TypeScript, so I'm not sure how to add a test for this or even if it would be needed... 😊

Would we also need to add a types property in the package.json file? Something like:

"types": "./psl.d.ts"

Can you please provide instructions on how to use/test this for a complete TS noob?

I'd be more than happy to accept the contribution, but would like to understand it first 🐵

Best,

L

@ntninja ntninja force-pushed the master branch 5 times, most recently from b916b03 to 2959ab3 Compare April 4, 2018 23:06
@ntninja
Copy link
Author

ntninja commented Apr 4, 2018

Sorry for the late reply! I've been updating the TypeScript file based on the recommendations here and here.

Would we also need to add a types property in the package.json file?

Indeed, and this has been added now.

Can you please provide instructions on how to use/test this for a complete TS noob?

You should be able to simply create a new JS project, add psl as a dependency in package.json and then add a index.ts that references the package using ES6 imports (import * as psl from "psl";) and use the API as usual. You can then transpile the TS to JS using the tsc command.
IMHO the real benefits of using TypeScript over JavaScript are mostly when using an IDE/TextEditor with auto-completion or when working on really large codebases. A 50 line JS snippet really doesn't need TypeScript at all.

I'd be more than happy to accept the contribution, but would like to understand it first

Understandable!

The unit test fail btw, because dtslint requires Node 6.10.x or higher. Do you know what to do about that.

@ntninja
Copy link
Author

ntninja commented Apr 5, 2018

ad testing: Currently the automated tests only check whether the .d.ts file is syntactically valid, but not whether it actually matches what is provided by the library its supposed to wrap. Unfortunately, there doesn't seem to be any good tooling for this, so the only real way of verifying this appears to be to write a regular unit test using typescript, transpile it with tsc and then let it run against the JavaScript library. Not ideal, but should work.

@lpellegr
Copy link

lpellegr commented Dec 5, 2018

Any progress?

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