Typescript

[EdJoPaTo]

This adds TypeScript types for the library. Even when not using TypeScript itself many IDEs read TypeScript type definitions in order to provide better autocompletion.

Is this something you would like to add?

I migrated two projects of mine to typescript while using this typedefs and had no problem yet.

Currently it is not complete, some simplify methods are missing.
I can add them when you think TypeScript type definitions are a great benefit for this project.

Also please check out a8b0dd4 as that is unrelated to typescript itself but fixes problems for me to work with this library. Is there something I am not seeing or is jsondepth simply not needed (anymore)?

[EdJoPaTo]

I dropped the not relevant commit from this branch via rebase, now the discussion is not linked anymore: a8b0dd4

[EdJoPaTo]

@maxlath whats your general thought? Do you like to add TypeScript Types? Should I type the rest in order to be able to merge this?

[maxlath]

I'm not very familiar with TypeScript yet, so I don't feel able to merge and promise it will be maintained on my own, but if you're ok to help maintain it, I don't see any downside in having them

[EdJoPaTo]

In general types are quite good now.

I think some types could be improved when needed as they are currently just any. Especially the simplify claims part has a bunch of any. But that can be done when they are actually needed by someone using typescript.

I think libraries in general can benefit from being written in TypeScript natively instead of just being typed. Also this library seems a bit confusing at parts as it tries to keep legacy code which makes code harder to read. Maybe consider a breaking release that cleans up legacy stuff and increases code readability. (Especially the simplify claims code raises some question marks.)

[noinkling]

Note that there are already some types available over at: https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/wikidata-sdk

VS Code (my editor) downloads them automatically for intellisense, but they're definitely not perfect, e.g. some methods aren't present and certain parts seem to make my intellisense slow to a crawl. Still, they might provide a good reference.

[EdJoPaTo]

@noinkling should definitely take a look there. I didn't check that out as I thought the first thing to do is trying to add typings to the project directly instead via the DefinitelyTyped workaround?

Looks like @kamontat added them there.

I will integrate whats missing into this PR.

[noinkling]

@EdJoPaTo Just as a word of warning, I'd suggest not copying the massive union type for language codes in https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/wikidata-sdk/def/type/language.d.ts

Those codes change relatively often and I strongly suspect it's what caused most of my intellisense slowdown. I think it would be best just to leave keys/values that use those codes typed as string.

[EdJoPaTo]

@noinkling yeah. I do not like that massive list either. Thanks for the advice!

[EdJoPaTo]

@noinkling would you like to check the current typings with a project of yours? As you seemed to used typescript it could be helpful to have a different pair of eyes.

I had no issues so far and only 3 places remain with any / unknown as a type.

This took way longer than I expected but hey: Now I know way more about the Wikidata API :)

[noinkling]

In addition to the other stuff, missing the two truthy claim helpers documented here: https://github.com/maxlath/wikidata-sdk/blob/master/docs/general_helpers.md#claims-helpers

[noinkling]

Sorry for the spam, I should have made them all part of the same review.

The rest seems pretty good as far as I can tell.

[EdJoPaTo]

Sorry for the spam, I should have made them all part of the same review.

@noinkling You can combine suggestions into one commit. But I had to add tests and proper return types. See a200372 ;)

[EdJoPaTo]

@noinkling what do you think about using ReadonlyArray here? (see Common Mistakes)

also remove the DefinitelyTyped typings has to be done later.

Other than that… I think its ready. Everything marked by @noinkling is added / fixed.

[noinkling]

Re: ReadonlyArray, I'm not experienced enough with TS to know if it might cause issues or not, but it seems like it should be ok?

I should probably clear up that my project isn't actually written in TypeScript. I just use the TS language service built into VS Code for intellisense, and there's also an option I can enable for it to check JS, and I can annotate types with JSDoc where required. All that combines to make sort of a half-assed version of TS, but it means my TS knowledge is limited - you likely have a better idea of these things than I do.

[EdJoPaTo]

I haven't used ReadonlyArrays yet too. For now I think the typings are ready enough and can further adapted over the time anyway.

So @maxlath can merge them. But I think all the changes should be combined into a single commit as this PR has grown into 40 commits.

[maxlath]

I rebased and, as suggested, squashed all those commits in one: f30b4aa . Can I force-push it to EdJoPaTo:typescript and merge it? Alternatively, I can simply merge that single commit into master, and close this PR, but let your branch untouched if you still want to keep the history

[EdJoPaTo]

If I remember correctly squashing together the PR is an option on GitHub when accepting the PR.

The history is not that important to me, the end result is ;)

[maxlath]

thanks @EdJoPaTo and @noinkling !! types are now part of v6.1.0!

[maxlath]

@EdJoPaTo @noinkling I just released v7.0.0 which came with some major breaking changes, notably, there are now 2 modules: the Wikibase instance agnostic wikibase-sdk, and wikidata-sdk, which is simply wikibase-sdk initialized with wikidata.org config. Could you have a look at how types definitions could be updated to fit this new configuration?

[MrXyfir]

A new issue should probably be opened since types are definitely broken now. In the meantime @maxlath, you should push a new version without the types key in package.json so as not to give people bad types automatically.

[MrXyfir]

#61

[EdJoPaTo]

I try to have a look at the types maybe today or tomorrow.
Looking at the Changelog the names of the time functions have changed which is a simple fix. If I remember correctly I tried not to add types for legacy aliases but I will check that.
With the renaming to wikibase-sdk and the config reuse I have to check how to handle this best. 🤔

@maxlath Have you thought about writing libraries with typescript (long term)? Especially with things like the claim simplification it could help to have more readable code which is type checked. :)

[MrXyfir]

+1 for converting to TypeScript