spencermountain / wtf_wikipedia

a pretty-committed wikipedia markup parser
https://observablehq.com/@spencermountain/wtf_wikipedia
MIT License
773 stars 129 forks source link

Type definitions? #280

Open RobRoseKnows opened 5 years ago

RobRoseKnows commented 5 years ago

Hiya, glad to have found wtf_wikipedia as it's making my life easier. I was wondering if adding type definitions for typescript is anywhere on the roadmap. Would make integrating with Typescript applications very nice.

RobRoseKnows commented 5 years ago

Currently for example, you can't import wtf from 'wtf_wikipedia' in TypeScript, as it has no typings. You need to const wtf = require('wtf_wikipedia').

spencermountain commented 5 years ago

hey Robert, yeah, i'd like to add typescript typings, and of course, i'd merge a pr with them.

i've tried a few times to support them, but it was unclear how best to host and maintain them.

The best solution is if the typings could be tested with a npm run test:types command, so i can get yelled at when i forget to update them.

If you, or anyone is interested in writing these, there's a api.json file with all the public methods, which should help atleast a little

cheers

RobRoseKnows commented 5 years ago

Yeah I'm working on type definitions now, not sure how to test them though, I'll look into it. Wish I saw api.json earlier though, I've been going off the source code lol. I'm pretty sure api.json is wrong about some things though, for example, json() returns an Object, not an Array.

spencermountain commented 5 years ago

ah, thank you! yeah, I'm sure it is wrong. please fix it!

Let me know if I can help, at all. Some of the options params especially, are pretty gross.

MrXyfir commented 5 years ago

@RobRoseKnows How far along are you? Could you share what you've got so far? For now, I'm using the following, just so TypeScript will quit complaining:

declare module 'wtf_wikipedia' {
  export function fetch(titles: string | string[]): Promise<any>;
}

I plan on filling it out as I go, so anything helps. If I get it all completed I'll submit a pull request here.

RobRoseKnows commented 5 years ago

@MrXyfir I think I finished everything, though I don't know how correct they are: https://github.com/RobRoseKnows/DefinitelyTyped/tree/master/types/wtf_wikipedia

When I have a chance I'm going to add tests and submit a PR.

MrXyfir commented 5 years ago

Great! I'll plug it into my project and see if I discover any issues with it.

Also that link doesn't work (thanks to the capital W): https://github.com/RobRoseKnows/DefinitelyTyped/tree/master/types/wtf_wikipedia

Edit: Found a few issues for fetch().

Edit 2:

  export function fetch(
    title: string,
    lang?: string,
    options?: any,
    cb?: any
  ): Promise<null | Document>;

  export function fetch(
    titles: string[],
    lang?: string,
    options?: any,
    cb?: any
  ): Promise<Document[]>;

Edit 3:

Anywhere an argument uses ... | null | undefined or similar is causing me problems when no argument is provided. This might be because I'm using TypeScript in strict mode.

Instead do something like this:

// from
html(options: object | null | undefined): string;
// to
html(options?: object): string;

Edit 4:

I disagree with Section['depth'] being private. It's quite a valuable piece of data to have, and as far as I'm aware, is not accessible any other way.

I've made a handful of tweaks so far. Should I make a pull request to your fork of DefinitelyTyped?

Edit 5:

wtf('wikitext') should return Document and options should be optional (?).

RobRoseKnows commented 5 years ago

@MrXyfit: Didn't really know about optional arguments as I'm new to typescript but yes, anywhere it's ... | null | undefined should be optional.

Section['depth'] is private because the method indentation() simply returns this.depth.

Go ahead and submit a PR if you can.

MrXyfir commented 5 years ago

@RobRoseKnows Ah, my bad. I somehow missed indentation().

I can't figure out the pull request, seems to only let me merge with the original repo and not yours. Maybe you can do it: https://github.com/MrXyfir/DefinitelyTyped/tree/master/types/wtf_wikipedia. If not, just copy and paste from it.

I haven't fixed all of the arguments that should be optional but I think the majority have been updated.

RobRoseKnows commented 5 years ago

So I merged in your changes and they look good, except now when I try to write the linting tests, I get an error, I think it has to do with the overloaded methods, but I'm not sure. I may just simplify the types so that there are no methods with identical names and see if that fixes it.

MrXyfir commented 5 years ago

Hmm. I think the only overloaded function I added was fetch(). You could use | for the titles and the return but that starts to be a pain when you use them in TypeScript (I think just strict mode) because then you have to do something like const doc = await wtf.fetch('test') as wtf.Document instead of it just automatically knowing the type. Might be easiest to just remove types for multiple docs altogether and then petition @spencermountain for a fetchAll([...]) or similar instead.

Let me know if there's anything I can do to help.

spencermountain commented 5 years ago

hey! yes, thank you. I agree fetch is too overloaded. The intentions, i assure you, were good! ;) I'll look at cleaning it up in future versions.

spencermountain commented 5 years ago

if you do fetch(any, any, any), i can adjust it once the method gets cleaned up. Because it will be a breaking change, it's behind a few other things.

MrXyfir commented 5 years ago

@RobRoseKnows Any update on this? Is there anything I can do to help?

I ended up removing the overloaded fetch() from my own copy of the types as it was causing me trouble and I didn't have any need to fetch multiple documents. Should be fine to just remove it for now if that's what you're stuck on.

RobRoseKnows commented 5 years ago

Yeah sorry. I don't really know how to integrate the typings into the test suite of wtf_wikipedia. Currently it's just a fork of DefinitelyTyped, which has its own test suite, but I don't really know how we can add it to what we already have here.

spencermountain commented 5 years ago

hey Rob, feel free to make a pr for these without a test thing. then we can all work on it together. prs welcome cheers

spencermountain commented 5 years ago

hey guys, I copy+pasted Rob's types into ./types on master in 7.6.0!

I'm not sure it's correctly implemented, but I just went for it. I've played-around in vscode and it pulls-in tooltips, and seems sane-enough.

Thank you so much for your help. I had no idea how much of an undertaking this was. I've been throwing so many weird overloads for several years!

one thing I haven't gotten working is the npm run test:types idea - I thought if i ran one token smoke-test with ts-node ./tests/types/index.ts, it would catch failed types.

ts-node must be more disciplined than how vscode pulls-in types:

TypeError: __1.default is not a function

can you guys double-check this, when you have a chance? thank you!

MrXyfir commented 5 years ago

@spencermountain Great!

A quick test on my end and the types seem to be working.

As for the error, I don't think that has anything to do with the TS types. Seems like an import error.

I think the issue is actually your tape import. Try it without the * as. Log both imports to console. Enabling TypeScript's esModuleInterop may help.

spencermountain commented 5 years ago

hahah, story of my life. will keep this open thanks.

ducpt-bili commented 2 years ago

hi @spencermountain , thanks for the greate lib. I am trying to use your lib on Nestjs project, but it seem like the typescript function is not work when i using the syntax:

spencermountain commented 2 years ago

hey @ducpt-bili nestjs looks neat , but i don't know anything about how it imports types. I think we're using best-practice on this side, but If you figure out something we can do, please let me know cheers

guyutongxue commented 2 years ago

FYI: You should set your tsconfig.json 's moduleResolution to node. Using Node16 or NodeNext will cause a "declaration file not found" error.