wikimedia-gadgets / types-mediawiki

TypeScript definitions for MediaWiki JS interface
GNU General Public License v3.0
23 stars 8 forks source link

Specify key/values with mw.Map #3

Closed cAttte closed 3 years ago

cAttte commented 3 years ago

this PR adds generics to Map, its methods, and one of its dependencies (mw.config). also, i moved it into a separate file because, imo, it doesn't really belong in the main mw namespace (and neither do html or log, which i will move in a separate PR).

i added this because the current model isn't really type-safe, which means you can do something like mw.config.get("aaa") without getting compilation errors (and get() will always return any, which is yucky). also, it defaults to any so existing code won't break

i got the types for the config from here, and also included the script i used to scrape them, but i couldn't really find a list for user.options and user.tokens, so that can wait for now!

siddharthvp commented 3 years ago

Type-checked mw.config is exciting!

However, is this working? I'm not sure if something is off at my end, but after defining let articlepath = mw.config.get('wgArticlePath'); articlepath is getting the type unknown rather than string.

Also, configs can be added by extensions. I'm getting an error on doing mw.config.get('pageTriageNamespaces') though it's valid on wikis with PageTriage extension enabled, such as the English Wikipedia. I think this can be fixed by adding an index signature [config: string]: any at the end of the list. This does mean that mw.config.get("aaa") won't raise an error, though.

cAttte commented 3 years ago

yeah, i probably should've tested my code first lmao. anyways, the unknown is caused by there being a fallback parameter with type any, which, when not provided, will cause the return value to be (config value) | any, so basically unknown.

i fixed this by just making two other overloads, so that if you don't provide a fallback parameter, the return value isn't affected by it, so it doesn't become unknown. so now it works:

![](https://i.imgur.com/0ggwih4.png) ![](https://i.imgur.com/q5vAMVP.png) ![](https://i.imgur.com/JBrRofq.png) ![](https://i.imgur.com/WNedNRC.png)

about the second thing, i didn't realize extensions could add config keys! i guess we can't really have a strict yet not annoying way of typing the config, then. so yes, [config: string]: any would be the best choice—after all, the greater advantage of this is having types for the keys that do exist, not detecting keys that don't (and editors like VS Code will still use these for autocompletion). and, there will always be more extensions, so typing everything isn't really an option.

however, wouldn't it be nice to type some of the most common ones? (e.g., PageTriage). like, i don't know if we would just include them in the default config, or add a way to select specific ones (which doesn't really sound possible with just TypeScript declarations)?

siddharthvp commented 3 years ago

Regarding the fallback param, would it make sense to require that the type of fallback is same as that of V[key] ? That is, the generic F would be unnecessary. Anyway not a big deal since mw.config.get() with fallback almost never gets used in reality.

siddharthvp commented 3 years ago

however, wouldn't it be nice to type some of the most common ones? (e.g., PageTriage). like, i don't know if we would just include them in the default config, or add a way to select specific ones (which doesn't really sound possible with just TypeScript declarations)?

Pretty unnecessary because the usually used configs are the ones present in MW core. Use of the ones added by extensions is quite rare in my experience. Also, there are too many of them to add. (go to https://en.wikipedia.org/ and run mw.config.values - turns up a heck lot of stuff.) As for type checking would it be better to use [key: string]: unknown rather than any. This way we'd actually get an error when using the value from mw.config.get('aaa') in a type-checked context. And things like mw.config.get('pageTriageNamespaces') can still be used with a cast.

That is,

let x = mw.config.get('pageTriageNamespaces') as number[]; // explicit cast required for non-core configs.
let x = mw.config.get('wgArticleID'); // Core config so expected to work without a cast, but wait that's a typo!
someFuncExpectingNumber(x); // error!
cAttte commented 3 years ago

yes, you're right, unknown is better for this case. i also removed the extra overload with F and just used V[S], as you said; it doesn't make a whole lot of sense to fallback with a different type. now its behavior matches get(string[])—which wasn't able to return something like Record<S, V[S] | F> because that's not how index access types work, so it'd just discard the fallback type. (though, if you select multiple keys with different types and choose a fallback, the type will be slightly wrong but we can't really do anything about it and that's a very edgy case).

siddharthvp commented 3 years ago

This looks great! Thanks. Your typescript-fu is clearly way better!

cAttte commented 3 years ago

haha i try, thanks for correcting my errors too!