Add Type definitions or convert project to TypeScript

sachaw commented 3 years ago

Hi, I have started using the meshtastic.js lib recently, but as a typescript developer I am having to generate my own type definitions. looking at the how the library is being written, it should be easy to convert to ts, which would allow for better es6 support and targeting js versions for the output, I would be willing to make this conversion and make a pr, just seeing what people's opinions are first.

thepoweroftwo commented 3 years ago

First of all, thanks for offering to convert the lib! We can consider switching to TypeScript, in my opinion there is no real disadvantage in doing so. In this case it would extend compatability of the library, because TypeScript can be transpiled to ES6 easily. Typing can always be stripped, but hardly added automatically.

If you want to do a pull request, it should take into account:

Compatibility with the used dependencies (protobufjs and EventSource polyfill)
Change the webpack build process to work with TypeScript (ideally we'd also output a pure es6 version then, not bundled, but that might be for later)
Compatibility with JSDoc or an alternative for automatized documentation generation
Following the existing coding patterns/style
Not increasing the final bundle size by a considerable amount (over 100KB)

Maybe you could give a quick overview of how you would implement the typing? (I am not a pro in TypeScript, but it should be all basically known object oriented principles i assume). For example, Protobufjs provides its own data structs and objects that you get back when a message is received, but right now when you want to set the config of the device, you can only provide a plain object with the members preferences and/or channelSettings. The protobufjs classes are not exposed to the outside of the library. Another thing to think of, would the interaction with meshtastic.js change whether one is using the TypeScript version or the bundled es6 version?

sachaw commented 3 years ago

Hi, Thanks for the reply, I have been playing around with a few ideas of the best way to implement this.

Firstly, as for the issue with defining Protobuf types, we can provide specific definitions for the structure of them, or as I have it for now, just allow it to assume any as the return type, until such a time as I write a type definition for them.

As for compatibility with protobufjs and EventSource polyfill they already have type definitions so they work out of the box.
As far as building goes, it's very flexible, I propose a structure, somewhat like the following.

dist ├── lib │ └── commonjs non bundled + types + sourcemaps ├── lib-es6 │ └── es6 non bundled + types + sourcemaps ├── meshtastic.js (bundled and minified)

In terms of bundle size, a pre-emptive output from webpack with ts-loader (unoptimized) had a minified file of exactly 100kb with room for improvement.
On your last note, as all of the outputs are built from the same source, usage would not differ between version and environments.

sachaw commented 3 years ago

What's your opinion on this optimization:

    async _readFromCharacteristic(characteristic) {

        try {
            let readBuffer = await characteristic.readValue();
            readBuffer = readBuffer.buffer;
            return readBuffer;

        }
        catch (e) {
            throw new Error("Error in meshtasticjs.IBLEConnection.readFromCharacteristic: " + e.message);

        }

    }

to:

    async _readFromCharacteristic(characteristic: BluetoothRemoteGATTCharacteristic) {

        try {
            return (await characteristic.readValue()).buffer

        }
        catch (e) {
            throw new Error("Error in meshtasticjs.IBLEConnection.readFromCharacteristic: " + e.message);

        }

    }

I find it's easier as type inference takes over and we don't have to manually type anything.

Side node: do you have a prettered eslint/prettier config you use, (my default one is different and there's no config in the repo)

sachaw commented 3 years ago

Just pushed some preliminary work to my fork sachaw/meshtastic.js I have ommited the built files (docs and minified js etc) as I suspect there will be quite a few changes before it gets merged. Feel free to have a look over it: seems to be working fine for me. Next few steps I think will be to continue improving the commenting and documentation also get type definitions for the protobufs.

sachaw commented 3 years ago

Just a suggestion: instead of building the documentation and committing it, use GitHub actions or Vercel to auto deploy it on every commit.

thepoweroftwo commented 3 years ago

Sounds great! I will take a look at it.

I find it's easier as type inference takes over and we don't have to manually type anything.

Sure, we can change that, it's equally or even better readable.

Side node: do you have a prettered eslint/prettier config you use, (my default one is different and there's no config in the repo)

Currently not, since I was transitioning to VSCodium at the time of writing the lib. I could configure one though. Or you do, if you want to.

Just a suggestion: instead of building the documentation and committing it, use GitHub actions or Vercel to auto deploy it on every commit.

Good idea. I'd like to keep the possibility to build the docs locally, as an additional way.

sachaw commented 3 years ago

Just a quick note, I added a prettier config (default) so you can run npx prettier --write . to lint all the files, or use the prettier plugin to do it automatically, I suggest we add this as a NPM script and maybe make it a precommit hook.

As for building docs, it will always be 100% possible to build them on demand

thepoweroftwo commented 3 years ago

From a quick look what you've done looks really nice!

Two things: Is there a reason why you removed the static before setDebugMode in SettingsManager.ts? And in meshdevice.ts there are the two methods readFromRadio and writeToRadio defined as public. I don't get why.

The next step would be to define all shared types from the protobufs, right?

sachaw commented 3 years ago

as for setDebugMode probs just overlooked it, ill add that back. as for the _readFromRadio and _writeToRadio methods in meshdevice.ts for now I had to define placeholder methods to get overwritten by ibleconnection and ihttpconnection because typescript could not detect that they were present when the class was extended, I'll look into a better way to do this, but it works for now.

as for the protobufs, I'll need to take a better look at the protobufsjs lib to work out a good approach

thepoweroftwo commented 3 years ago

for now I had to define placeholder methods to get overwritten by ibleconnection and ihttpconnection because typescript could not detect that they were present when the class was extended

I see, that's not a problem. I think the way to go would be to define imeshdevice as an abstract class: https://www.typescriptlang.org/docs/handbook/classes.html#abstract-classes And define the readFromRadio and writeToRadio as abstract properties in there, that must be implemented by children.

Protobufs: One option would be to model all protobuf types as own interfaces in staticTypes.ts. But since protobufjs already provides them, could we just import these types wherever we need it in the code? Thinking about pros and cons here..

sachaw commented 3 years ago

yeah, that sounds good to me, and thanks for the docs, was being lazy and not wanting to dig through and find how to do it correctly, I'll have a fair bit of time tomorrow(I'm in UTC+10) to work my way through the bulk of it .

sachaw commented 3 years ago

Give this a go https://github.com/sachaw/meshtastic.js/commit/f7d5992330278771c1f366b1b9d2a6d0b045b173

thepoweroftwo commented 3 years ago

Looks good. I'll try to test it later.

The async/await structures should not be removed though. For instance in configure() or in sendPacket(), the readFromRadio and writeToRadio must be awaited, otherwise the function execution continues before the promise is resolved.

Async/Await is just syntactic sugar for Promises with .then. Everything below await essentially gets packed into .then() and is executed when the promise resolves.

sachaw commented 3 years ago

I'll have to look into the typings then, as all the methods I removed the awaits from weren't typed as promises

thepoweroftwo commented 3 years ago

You mean the methods that were awaited or the methods that contained the await were not typed? From my understanding, you'd type every async method like that: : Promise<Type> where <Type> is the type the promise resolves to.

And in the method where an async func is awaited, the await would return the resolved type, not a promise. But in the cases where you removed the await, the return type/content is not important, we just need to wait until the promise is resolved (with await) and then continue with executing the rest of the method. Maybe I'm missing something..

sachaw commented 3 years ago

my bad, before correctly typing the class abstracts, they had an any return type instead of a promise, so they were auto removed, fixed https://github.com/sachaw/meshtastic.js/commit/c526b344f7a40806a86c03b23a6e7b38bfd75f33 also any idea what this is for https://github.com/sachaw/meshtastic.js/blob/c526b344f7a40806a86c03b23a6e7b38bfd75f33/src/ihttpconnection.ts#L232

sachaw commented 3 years ago

Wherever you have a function that will resolve without any output there just typed as Promise<void>

thepoweroftwo commented 3 years ago

Ah, our posts overlapped ;) Alright.

The line you linked just triggers the readFromRadio, where the http API is fetched. readFromRadio in turn calls handleFromRadio. So the let r can be removed.

sachaw commented 3 years ago

Take a look @ https://github.com/sachaw/meshtastic.js/commit/65042f49ef4eeb1eb27a603d92464994d1f2e844 there are still a number of things that need investigating/deciding on (all annotated with @todo). Also, The current compiled build is ~90KB I'd also like to look at diectly parsing the .proto files if at all possible: protobuf.load("mesh.proto", (_, __) {} as it would eliminate a whole file. Lastly, I need to work out the correct way to type oneof protobuf types and the clash between Type & protobuff.Type

thepoweroftwo commented 3 years ago

Ok, here are some starting points for protobuf types: https://github.com/protobufjs/protobuf.js#using-custom-classes https://github.com/protobufjs/protobuf.js#using-generated-static-code

Also, protobufjs has 2 cli tools: pbjs and pbts. pbts can generate .ts files from an annotated .js file. Don't know how that works yet.

As for parsing the .proto directly: Also possible, but that requires the full protobufjs library to be included, which increases the overall size. Also it's possible to generate completely static code from a proto file with pbjs. (That also takes more space than parsing the json).

Maybe another option: We change the protobufhandler.ts to convert from the protobufjs object types to our own interfaces. (The ones you already defined) That way, we have more control over the types, because to the rest of the library only our defined types are exposed. That wouldn't solve the Type problem in protobufhandler.js though. But maybe it's possible to generate typings with the cli tools.

sachaw commented 3 years ago

Sounds like a good idea, I tried pbts, but didn't output anything useful. I have ordered 3 new TTGO T-Beams to test on (don't have any hardware on me atm) that should be here within the week. Later down the track, It would be good to implement at least some basic tests. There is support with jest for web-bluetooth.

sachaw commented 3 years ago

@thepoweroftwo Any chance you can do some preliminary tests to ensure nothing major is broken, As I still can't test certain functionality few a few more days

thepoweroftwo commented 3 years ago

Sure, i finally took the time to do that now. It seems to work fine, except sending a Text, when I get:

meshtastic.js:formatted:3961 Uncaught (in promise) TypeError: Cannot set property 'payload' of undefined
    at EventTarget.sendData (meshtastic.js:formatted:3961)
    at EventTarget.sendText (meshtastic.js:formatted:3955)
    at app.js:45

The error seems to be around these lines in sendData():

    let data: Data;

    data.payload = byteData;
    data.typ = dataType;

I can't tell from a quick look what's wrong there.. should work? Aside from that, connecting to device, configuring and receiving messages works just fine.

Edit: I overlooked that the variable had an interface type assigned. I changed the code to:

    let data: Data = {
      payload: byteData,
      typ: dataType
    };

Now i get the error Error in meshtasticjs.ProtobufHandler.toProtobuf:packet.decoded.data.typ: enum value expected Has sth to do with the enum type/number that protobufs returns. It seems that we just have to use our internal enum type here. Protobufjs should parse that correctly to a protobuf.

sachaw commented 3 years ago

Great, thanks for that, I'll take a look at that tomorrow. When I get my new Tbeams, I'll start fixing the protobuf types, protobufjs supports decorators, so it will be fairly easy.

sachaw commented 3 years ago

@thepoweroftwo Have made a lot of headway with the protobuf implementation, for an example, a basic packet to send to a radio would be:

let toRadioData = new ToRadio({
  packet: meshPacket
})

let encodedData = ToRadio.encode(toRadioData).finish()

await this._writeToRadio(encodedData);

which can certainly be condensed further, also encoding:

fromRadioObj = FromRadio.decode(fromRadioUInt8Array)

because we're using decorators, we can instantiate, encode and decode any data type that we define.

I've still to work out how we want to handle errors emitted form any of these methods as there will no longer be a protobufhandler

sachaw commented 3 years ago

There may certainly be issues with it, (new TBeams still haven't showed up yet, so cant test) but transpiles fine for me https://github.com/sachaw/meshtastic.js/commit/c1c8c100431eaf3b81eb347474668076b956102b

thepoweroftwo commented 3 years ago

That's a clean solution for the protobufs, I like it. Wouldn't the .encode and .decode functions throw an Error if they fail? Then we can just try catch those functions. Ah, hmm.. TypeScript doesn't do runtime type checks, right? What happens if one tries to send a meshpacket with wrong data in it?

sachaw commented 3 years ago

Thanks, I also like now how the docs will generate info on all the methods for each ptotobuf type/interface. As for verifying, We can add a verification step for each of these, but as we are programmatically generating the data that is encoded, the user should not be able to generate data that is incompatible. Also, as we are mostly checking the data that comes in and is parsed, in many cases, verifying before encoding would be redundant.

sachaw commented 3 years ago

Also, many functions such as connect, disconnect return 0 on exit, also addNode, addUserData etc. Return the modified node id. I believe all of these should return void as it is redundant and inconsistent, in the case of returning the node id, as it is required to be passed to the function in the first place, the user should have access to it anyway

thepoweroftwo commented 3 years ago

Also, as we are mostly checking the data that comes in and is parsed, in many cases, verifying before encoding would be redundant.

Right.

I believe all of these should return void as it is redundant and inconsistent

Also correct. There are some inconsistencies that should be streamlined. Not sure why I chose to do that, lol.

sachaw commented 3 years ago

This is the list of things I'm looking to do before I submit a PR, please feel free to add any more.

unify and strongly type connect definition
rewrite/improve _dispatchInterfaceEvent
improve comment coverage and consistency
test all functions and record methods of testing for later implementation of unit tests probs a few more minor ones

thepoweroftwo commented 3 years ago

What do you think, would it make sense to

Export the types from protobuf.ts in index.ts

so that one using the lib in typescript could easily import them?

Also, right now private class functions are named with a preceding _. We could actually make them private in TypeScript, so that it is clear they are not meant to be called from outside.

Aside from that I have nothing to add :)

sachaw commented 3 years ago

Also, right now private class functions are named with a preceding _. We could actually make them private in TypeScript, so that it is clear they are not meant to be called from outside.

https://github.com/sachaw/meshtastic.js/commit/54fbc59569afcc49ce4e541f19a55e92647ac227 May want to privatize/publicize more, but that's just a 1:1 conversion with the exception of any abstracted functions which cant have limited visibility.

sachaw commented 3 years ago

As for exporting the classes form protobuf.ts If they are using an ide that has type detection, they will already have full type support, I can however export all the classes so they can decode and encode data in any of the supported formats, however I see no need to do that as we handle all of the encoding end decoding and the user has no way to pass this data to a device, or receive raw data to decode.

thepoweroftwo commented 3 years ago

I see, then we don't need to export them again. Was still stuck in the ES6 class way of thinking...

Edit: Btw, do you know about the slack meshtastic dev team? If you want to join: https://meshtastic.discourse.group/t/meshtastic-development-live-chat/623

meshtastic / js

Add Type definitions or convert project to TypeScript #5