kaitai-io / kaitai_struct_javascript_runtime

Kaitai Struct: runtime for JavaScript
Apache License 2.0
37 stars 22 forks source link

Port JavaScript runtime to TypeScript #18

Open vogelsgesang opened 4 years ago

vogelsgesang commented 4 years ago

From PR #10, I understood that @GreyCat is generally open to moving over this whole library to TypeScript, as long as we don't break compatibility.

Since I am interested in using strict TypeScript types for my own project built on top of Kaitai, I went ahead and prototyped how this move might look. Doing so, I came across the following issues:

I already locally validated that the test suite still works with this commit.

However, I guess this commit isn't quite ready to be merged, yet. E.g., I am not sure what to do about the test suite integration and if we want to really use rollup.js or if there is a more low-tech way to get a proper UMD preamble. I would appreciate some guidance on how to further proceed with this :)


This commit ports the JavaScript runtime to TypeScript.

The main work of this commit is to port the KaitaiStream.js to KaitaiStream.ts. This was pretty much straight search-and-replace task to map the previously prototype-based class definition to a TypeScript class definition.

Only in one case, the mapping was not straigtforward: The deprecated readBitsInt was previously set to the exact same function instance as readBitsIntBE. This is not possible in TypeScript. Instead, this readBitsInt is now implemented as a separate function which simply forwards all calls to readBitsIntBE.

This commit does not yet add types to the class member and function arguments. I am planning to add strict types in a follow-up commit.

To compile the TypeScript file to a JavaScript file, this commit uses rollup.js. I would have prefered to directly use the TypeScript compiler, however the TypeScript compiler's UMD loader is missing the "global" fallback in case no module loader is available.

Usually, I would put the "Kaitaistream.js" into the .gitignore because it is a generated file. However, doing so would break the existing super-project structure, because the run-javascript script inside the tests repository does not actually build the JavaScript runtime but only links it

generalmimon commented 4 years ago

Usually, I would put the "KaitaiStream.js" into the .gitignore because it is a generated file. However, doing so would break the existing super-project structure, because the run-javascript script inside the tests repository does not actually build the JavaScript runtime but only links it

"Reluctance to change the current build system" is not a sufficient argument to track it with Git. Tracking generated files with VCS should be generally avoided whenever possible, as it brings many problems for minimal gain. All that needs to be done to build KaitaiStream.js here is npm ci && npm run build AFAIK.

So please add KaitaiStream.js to .gitignore.

And to what you wrote, run-javascript isn't supposed to build the JavaScript runtime, even after merging this PR. Scripts matching run-* in the tests repo are for development. They are intended to be run multiple times on the same computer. Therefore, run-javascript script can assume that the programmer has set up a working environment (e.g. the runtime library is ready). It would be a waste of time to build it on every run.

Well, if we want the run-javascript script to be perfect, it could ideally run the npm ci && npm run build if it detects that the file KaitaiStream.js doesn't exist, or if KaitaiStream.js was last modified earlier than KaitaiStream.ts (by querying the file system for last modification timestamp).

That's pretty advanced stuff though, and it doesn't have to be. Just leaving the runtime library build management to the programmer is OK too.

However, it's important to ensure that it doesn't break the CI. This means that it'd be either necessary to add npm ci + npm run build commands to the prepare-javascript script which is run prior to every CI JavaScript run.

Or, for more comfort and luxury, set up a Git post-checkout hook which executes the commands on every checkout. This would ensure that git clone is everything you need to get the runtime lib up and running :-)