Validate existing JavaScript implementation with TypeScript

EzraBrooks commented 7 months ago

Public API Changes

The internals of some class constructors no longer protect against users passing undefined or no argument for options. However, this was never actually supported, so this should not affect the great majority of users.

Description

Using the TypeScript compiler to statically type-check the existing JavaScript in this repository (and hopefully also auto-generate some typescript typings, that would be nice)

TODO:

[x] get it building without errors
[x] add the typescript step to the grunt build
[ ] Tweak some build settings, probably, to reduce compiled size (removing unnecessary polyfills)

EzraBrooks commented 7 months ago

Refactored prototypes to classes in 98ab37c due to TypeScript lacking support for JSDoc @extends for prototypes.

EzraBrooks commented 7 months ago

Current state of things:

Found 107 errors in 18 files.

Errors  Files
     2  src/actionlib/ActionClient.js:21
     1  src/actionlib/ActionListener.js:21
     4  src/actionlib/Goal.js:17
     7  src/actionlib/SimpleActionServer.js:19
     2  src/core/Param.js:14
    38  src/core/Ros.js:69
     7  src/core/Service.js:15
     3  src/core/SocketAdapter.js:15
    12  src/core/Topic.js:18
     1  src/math/Pose.js:27
     1  src/math/Vector3.js:45
     2  src/node/SocketIO.js:8
     3  src/node/TopicStream.js:14
    14  src/tf/TFClient.js:21
     1  src/util/cborTypedArrayTags.js:67
     1  src/util/decompressPng.js:18
     2  src/util/shim/decompressPng.js:8
     6  src/util/workerSocket.js:2

EzraBrooks commented 7 months ago

every time I make the JSDoc comments more accurate the compile errors get worse 😛

Found 108 errors in 20 files.

Errors  Files
     3  src/actionlib/Goal.js:47
     6  src/actionlib/SimpleActionServer.js:87
     2  src/core/Param.js:67
     2  src/core/Ros.js:69
    10  src/core/Service.js:53
     3  src/core/SocketAdapter.js:15
    27  src/core/Topic.js:72
     2  src/node/SocketIO.js:8
     4  src/node/TopicStream.js:14
    10  src/tf/TFClient.js:90
     1  src/urdf/UrdfBox.js:23
     1  src/urdf/UrdfColor.js:17
     2  src/urdf/UrdfCylinder.js:19
    10  src/urdf/UrdfJoint.js:34
     3  src/urdf/UrdfModel.js:36
     1  src/urdf/UrdfSphere.js:19
    13  src/urdf/UrdfVisual.js:43
     1  src/util/cborTypedArrayTags.js:67
     1  src/util/shim/decompressPng.js:8
     6  src/util/workerSocket.js:2

EzraBrooks commented 7 months ago

Thinking further on 8e09b7509c71eef1df8797b414ad52ffa5250a25, I suspect the intent of the options = options || {} is to avoid runtime errors from accessing named fields on an undefined variable, not that options is literally optional, even though that is what it kind of does.

We probably need to make a decision around the intent there, because that line is not gonna play well with static type checking. Either we remove it and make options mandatory which might technically be a breaking change for some very niche edge cases, or we go with my current solution of making everything optional which is.. gonna result in a lot of conditionals.

EzraBrooks commented 7 months ago

The tsc build is passing! I had to scatter more ts-expect-error directives around than I would've liked, but that's why it's there - to allow slow conversion from untyped to typed code.

EzraBrooks commented 7 months ago

I'll try to get CI passing and get this integrated with Grunt before I leave for my Thanksgiving vacation end of day today.

EzraBrooks commented 7 months ago

When reviewing this I highly recommend checking the "hide whitespace" box because there's a ton of indentation changes because of the prototype->class migration.

EzraBrooks commented 7 months ago

I've clearly mishandled at least one null case somewhere that will need to be tracked down before this will pass the example tests...

EzraBrooks commented 7 months ago

Okay, it's starting to look unlikely that I'll finish this before I head off on a week of vacation for Thanksgiving, but it should be a matter of "just" fixing the tests at this point. It's probably something silly..

EzraBrooks commented 7 months ago

I've been progressively rolling back any "fixes" that I made in the name of making red squiggles go away, in favor of instead marking them with // @ts-expect-error so we can have the build pass but have a to-do list of fixes to make by just searching for that marker in the codebase.

This PR is a substantial enough improvement just from having syntactically-correct JSDoc that is validated by tsc and having ES6+ classes.

EzraBrooks commented 7 months ago

Okay, this is passing tests. I'm out for the next week on vacation. @sea-bass, if this would make your life easier by obviating the need to update DefinitelyTyped for the ROS 2 Actions support, please feel free to rebase this over your changes and enable "declaration": true, in tsconfig.json.

sea-bass commented 7 months ago

Okay, this is passing tests. I'm out for the next week on vacation. @sea-bass, if this would make your life easier by obviating the need to update DefinitelyTyped for the ROS 2 Actions support, please feel free to rebase this over your changes and enable "declaration": true, in tsconfig.json.

I got this all rebased and working, but somehow when I link to my updated commit I get the error:

 ERROR(TypeScript)  Could not find a declaration file for module 'roslib'. '<MY_PATH>/node_modules/.pnpm/github.com+RobotWebTools+roslibjs@ce4a9901e79e3d71e72df59774191740a3820751_al2pucohqk5r54hjtllxqa5dsu/node_modules/roslib/src/RosLibNode.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/roslib` if it exists or add a new declaration (.d.ts) file containing `declare module 'roslib';`
 FILE  <MY_PATH>/src/utils/communication/roslib-utils.tsx:1:48

  > 1 | import { Topic, Message, Param, Service } from "roslib";

... and yes, I did change to use "declarations": True" in the tsconfig.json, as shown in the commit hash above.

Any ideas?

EzraBrooks commented 7 months ago

getting very close but still wrestling with some JSDoc details.

EzraBrooks commented 7 months ago

I've been playing with the typescript compiler settings and I don't think I can make the bundle smaller, if we're going to continue supporting ES5 (which I think we should for now).

In my opinion we should take the size hit, then switch to a more optimal bundler that may help, then over time incrementally raise our target ECMAScript version to reduce the number of polyfills TypeScript is including.

EzraBrooks commented 7 months ago

I think we should get rid of the built content in the repo a la #643 before merging this - but yeah, need v2 cut.

EzraBrooks commented 6 months ago

consensus between me and @sea-bass: this is:

a release delineating change
too big to review well
and I really did try pretty damn hard not to break API which was quite difficult

so we're gonna merge this and fix any other problems as they arise in our usage or in Issues.

sea-bass commented 6 months ago

We also are actively using this branch in our project, so that gives us confidence that it does, in fact, work.

MatthijsBurgh commented 6 months ago

Why are we bumping to 1.4.1? Why not already bump it to 2.0.0, so people know this isn't 1.X anymore. Doesn't mean we need to release it.

MatthijsBurgh commented 6 months ago

Or this isn't breaking yet?

sea-bass commented 6 months ago

Why are we bumping to 1.4.1? Why not already bump it to 2.0.0, so people know this isn't 1.X anymore. Doesn't mean we need to release it.

We probably should bump it to 2.0.0. I can do it in https://github.com/RobotWebTools/roslibjs/pull/645, or we can do it as a standalone PR.

EzraBrooks commented 6 months ago

this shouldn't be breaking yet - but anyway, sorry, the 1.4.1 came in as a merge conflict and I should've reconciled it as 2.0.0!

RobotWebTools / roslibjs

Validate existing JavaScript implementation with TypeScript #649