Add JSDoc comments to function calls

[rikkuness]

Apologies if this is too overreaching or not something that's desired. Ended up down a slight tangent. 😄

While working through and understanding the codebase better to learn how to best implement #188 I started to add JSDoc comments to some of the function calls just to make it easier working through call signatures in an editor.

Most of what's there was comment blocks that already existed, just reformatted into a JSDoc type standard so they're picked up along with types expected etc.

Ultimately this can also mean Typescript definitions can be auto generated from the JSdoc comments, if that's something you wanted.

I'll keep this one open and WIP, and if it is something you do want adding I can slow time work through the rest of the call signatures and add to them.

[gfwilliams]

Thanks! In theory yes, I'm all for this. What I don't want is too much of:

/**
This checks if the Foo is a Bar
@param {*} a foo
@returns {*} is Foo a Bar?
*/
function isFooBar(foo) {...}

Someone did this to the Espruino codebase a while back and been a complete pain - it's just one more thing to maintain and possibly go out if date.

Changing existing comments over to /**, or adding comments that provide more info than you could work out from the function name is great. I'm less convinced about generic @param/returns

Adding types could be handy - do we know if eslint can use those to help with linting? IMO that'd be the real reason for adding them, if it could be used to pick up problems.

Don't know how you want to work with this - shall I merge now, or you want to wait until there's more?

[rikkuness]

Yeah that was my concern, I can tell it's a pretty elegant low dependency codebase so wasn't sure you'd want an excessive level of fluff, the JS ecosystem can become unnecessarily complex.

The @param and @returns would be the things that type hint any typing generation etc. so they're probably more useful in functions that are end user facing rather than every internal function, especially in cases as you demonstrated where it's faily obvious what it is/does. It's kind of a poor mans, or light touch, Typescript'ing 😄 That's made a bit harder with the Espruino global though for example, as the end user implementation is kind of hidden away behind the init functions etc.

This example below shows the new hinting for the index.js when the module from NPM is loaded. Helpful as an end user at least to know what the callback should look like etc. as the type hinting is there for what each param expects, more helpful in some use cases than others though I agree.

It's probably not something ideally suited to linting, I think to get those kinds of type safety checks to make sure correct types are being passed through the whole, you'd probably be best placed making the files full .ts files and then transpiling back to JS on publish to NPM. Which I personally like, but I can totally understand the aversion to that too, it's a whole thing 😂

I wouldn't necessarily merge this yet though, it was more to gauge your thoughts, there's places it could help and there's places where yeah it might be more maintenance for you in the long run which I wouldn't want to cause. It did turn up a few things though like here where the input param should actually be an ArrayBuffer probably, but the param is named str which is never used and the UintArray calls for a variable buf which doesn't exist, so not sure those functions ever worked https://github.com/espruino/EspruinoTools/blob/master/core/utils.js#L818 which to be fair, the linter also picked up on with the no-undef and unused vars checks 😄

[gfwilliams]

Thanks - for index.js that kind of help when using the module is awesome (including the params).

Honestly, it sounds like we're on the same wavelength about this - so it's up to you. If it's helpful to you, especially if you're making the changes anyway I'm very happy to merge them in.

Thanks for the arrayBufferToString hint - just fixed it! Looks like it's not used as you say (just in a commented out print statement) but I'm very happy you're poking through the codebase spotting this stuff!

[rikkuness]

Fantastic 👍🏻 Well if you're happy with the approach, leave this open for now while I work through the rest of the codebase, I'll comment JSdoc where feels appropriate and then I'll ping you once this is out of WIP and see where we're at?

[rikkuness]

So as it turns out there is a checkJs option.

Enabling it shows 210 errors with typing though 😂 There are definitely some legitimate bugs it turns out but like with the linting it'd be sifting through all the less serious things to find the bugs 🤷🏻‍♂️

[gfwilliams]

So as it turns out there is a checkJs option.

Great find - thanks! At some point it'd be good to have the ability to npm typetest or something even if we don't put it in GitHub actions - as you say it might throw up some issues amongst the noise - maybe over time we can find ways to trim down the amount of errors it creates.

[rikkuness]

At some point it'd be good to have the ability to npm typetest or something

I've added it as npm run ts:validate.

I've also added npm run ts:generate to generate the Typescript definitions in types/, currently there's only the types for index.js added in there as those are the only things really exposed by the Node.JS module. It's quite hard to properly automatically type the things that are set as globals.

Going to call this one ready though in the current state, I don't want to make the diff massive, hopefully most of the changes here are just comments, minor lint fixes or utility stuff and nothing in the actual data path as the code that would make it a risky change.

[gfwilliams]

Looks great! Thanks for tidying up the { placements too

Merging now :)