octet-stream / dinky

JavaScript bindings for Philomena JSON API. Supports sites such as Derpibooru and Furbooru.
https://www.npmjs.com/package/dinky.js
MIT License
7 stars 3 forks source link

Add typescript typings #7

Closed Veetaha closed 4 years ago

Veetaha commented 4 years ago

There are still some TODOs to resolve @octet-stream demo This also means response types maintenance burden tho...

codecov[bot] commented 4 years ago

Codecov Report

Merging #7 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master        #7   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines          208       208           
=========================================
  Hits           208       208           
Impacted Files Coverage Δ
lib/Search.js 100.00% <ø> (ø)
lib/Tags.js 100.00% <ø> (ø)
lib/util/castDates.js 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update bf560fd...5e05bf5. Read the comment docs.

octet-stream commented 4 years ago

Hey, this is actually what I want to add to my library :D But since I’m not familiar with TS, I decided to write JSDoc instead, as you probably noticed. Also, I thought it would be enough for auto completions. At least it works for me when I’m writing this library and tests in VSCode. Anyway, thank you for the PR! I will take a look tomorrow.

Veetaha commented 4 years ago

I recommend TypeScript, it trades a bit more symbols to write (which in fact you already do even more verbosely with JsDoc comments :D) for consistency and static reliability to the codebase. I really can't write JS anymore (well I am no longer actively using TS though), but believe me, it saves a lot of overhead (when used with strict configuration). E.g. the dreaded "Cannot read property of null | undefined" is fully solved by TypeScript, because you have to explicitly say null | T if you mean that the value can be null and the compiler will bite you if you don't handle null value when you use it.

You can play with this by creating a sample project

mkdir test-dinky
npm init -y
npm install ~/dev/dinky
npm install -D typescript
npx tsc init
touch main.ts

And then try compiling this code in main.ts

import dinky from "dinky.js";

async function main() {
    const res = await dinky({}).comments();
    const reason = res.comments[0].editReason;
    const lowercasedReason = reason.toLocaleLowerCase();
    // if (reason !== null) { // notice that types are contextual, so after this check reason is not nulllable
    //     const lowercasedReason = reason.toLocaleLowerCase();
    // }
}
~/junk/test-dinky $ npx tsc
main.ts:6:30 - error TS2531: Object is possibly 'null'.

6     const lowercasedReason = reason.toLocaleLowerCase();
                               ~~~~~~

Found 1 error.
octet-stream commented 4 years ago

Man, I have tried TS and I know what it is and how it works. : D By "not familiar" I meant that I haven't learned it, since I don't see any reason for me. But too many people use it, so it would be nice to have typings here in my library. For me, it gives only one benefit – I have autocompletion in VSCode which means I don't need to open README every time I want to write something (such as tests) for my library : D IntelliSense works with JSDoc just fine so that's why I did improvements for in code documentation.

Is the PR ready for a review or you will add something else first?

Veetaha commented 4 years ago

It's pretty much ready, I marked it WIP until there are TODOs you would like to resolve

Veetaha commented 4 years ago

@octet-stream this is ready to merge unless you have some ideas for improvement

octet-stream commented 4 years ago

Well, if everything is correct, I can merge this. But I would like to have tests for these typings if necessary.

octet-stream commented 4 years ago

I've tried this thing before in my other project, but it didn't worked well for me, when I decided to move different types to different files: https://github.com/SamVerschueren/tsd

Perhaps I did something wrong, doesn't matter.

octet-stream commented 4 years ago

Hey, if you have no much time to write tests for typings, I can just merge it anyways and add them later. Either way everything else LGTM.

Veetaha commented 4 years ago

Regarding last commits here is the explanation otherwise it would be a bug to

import { Response } from "dinky";
const res = new Response();
Veetaha commented 4 years ago

Hey, if you have no much time to write tests for typings, I can just merge it anyways and add them later. Either way everything else LGTM.

Probably, you a right, I don't have enough time...

Veetaha commented 4 years ago

Hmm, I just noticed I didn't export Response initially, are you sure we should export it?

octet-stream commented 4 years ago

You didn't? Then it was my mistake :D

Veetaha commented 4 years ago

Okay, you fix it ;d

octet-stream commented 4 years ago

Ok, done :D

octet-stream commented 4 years ago

Okay, everything seems ok, so I'll merge it. Thank you so much!

octet-stream commented 4 years ago

I will publish this changes in a next release as soon as I add the tests.

Veetaha commented 4 years ago

Some day you've got to adopt types and testing them won't be a thing)

octet-stream commented 4 years ago

As long as I do small things, I don't really need typings) But since I've switched to VS Code from Sublime Text I want IntelliSense work correctly, so now I use JSDocs :D