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 typings #7

Merged
merged 15 commits into from
May 22, 2020

Conversation

Veetaha
Copy link
Contributor

@Veetaha Veetaha commented Apr 30, 2020

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

@codecov
Copy link

codecov bot commented Apr 30, 2020

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.

@Veetaha Veetaha marked this pull request as draft April 30, 2020 23:07
@octet-stream
Copy link
Owner

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
Copy link
Contributor Author

Veetaha commented May 1, 2020

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
Copy link
Owner

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?

@octet-stream octet-stream self-assigned this May 1, 2020
@octet-stream octet-stream added enhancement New feature or request typescript TypeScript support labels May 1, 2020
@Veetaha
Copy link
Contributor Author

Veetaha commented May 1, 2020

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

@octet-stream octet-stream self-requested a review May 1, 2020 13:51
lib/index.d.ts Outdated Show resolved Hide resolved
lib/index.d.ts Outdated Show resolved Hide resolved
lib/index.d.ts Outdated Show resolved Hide resolved
lib/index.d.ts Outdated Show resolved Hide resolved
lib/index.d.ts Outdated Show resolved Hide resolved
lib/index.d.ts Outdated Show resolved Hide resolved
@Veetaha Veetaha marked this pull request as ready for review May 10, 2020 21:40
@Veetaha
Copy link
Contributor Author

Veetaha commented May 10, 2020

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

@octet-stream
Copy link
Owner

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

@octet-stream
Copy link
Owner

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.

lib/Dinky.d.ts Outdated Show resolved Hide resolved
@octet-stream
Copy link
Owner

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
Copy link
Contributor Author

Veetaha commented May 22, 2020

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

import { Response } from "dinky";
const res = new Response();

@Veetaha
Copy link
Contributor Author

Veetaha commented May 22, 2020

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
Copy link
Contributor Author

Veetaha commented May 22, 2020

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

@octet-stream
Copy link
Owner

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

@Veetaha
Copy link
Contributor Author

Veetaha commented May 22, 2020

Okay, you fix it ;d

@octet-stream
Copy link
Owner

Ok, done :D

@octet-stream
Copy link
Owner

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

@octet-stream octet-stream merged commit cdc2872 into octet-stream:master May 22, 2020
@octet-stream
Copy link
Owner

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

@Veetaha
Copy link
Contributor Author

Veetaha commented May 22, 2020

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

@octet-stream
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request typescript TypeScript support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants