Port JavaScript runtime to TypeScript #18
Conversation
KaitaiStream.ts
Outdated
| }; | ||
| } | ||
|
|
||
| export default KaitaiStream; |
There was a problem hiding this comment.
just pushed an udpated commit which fixes this, here and in rollup.config.js
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
"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 So please add And to what you wrote, Well, if we want the 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 Or, for more comfort and luxury, set up a Git |
| super(); | ||
| // Workaround https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#extending-built-ins-like-error-array-and-map-may-no-longer-work | ||
| Object.setPrototypeOf(this, KaitaiStream.EOFError.prototype); | ||
| this.name = "EOFError"; | ||
| this.message = "requested " + bytesReq + " bytes, but only " + bytesAvail + " bytes available"; |
There was a problem hiding this comment.
The old code was setting the this.message manually because it didn't call the Error constructor for constructing the derived error object (only as a helper to get the stack trace by doing new Error().stack, but that doesn't really count). However, when you're calling super() here already, I think it's better to pass the error message into the Error constructor and let it set the this.message automatically.
This has an additional benefit of that the super constructor can add some information to the message. In Java runtime, this is used for reusing parts of the error message of validation errors. There's a common ancestor for all validation exceptions called ValidationFailedError, which starts like this (KaitaiStream.java#L580-L582):
public static class ValidationFailedError extends KaitaiStructError {
public ValidationFailedError(String msg, KaitaiStream io, String srcPath) {
super("at pos " + io.pos() + ": validation failed: " + msg, srcPath);Derived classes like ValidationNotEqualError are calling this ValidationFailedError constructor, so they can contain only the info specific for the particular validation, and leave the common parts to the super constructor (KaitaiStream.java#L604-L606):
public static class ValidationNotEqualError extends ValidationFailedError {
public ValidationNotEqualError(byte[] expected, byte[] actual, KaitaiStream io, String srcPath) {
super("not equal, expected " + byteArrayToHex(expected) + ", but got " + byteArrayToHex(actual), io, srcPath);Therefore, the final ValidationNotEqualError message looks like this:
"/seq/1: at pos 7: validation failed: not equal, expected [62 61 72 7c], but got [62 61 72]"However, please don't introduce the error hierarchy in this PR, stick to the current JS approach. Let's proceed in small steps. After this PR is merged, you can create another PR with that, or I can commit it myself.
| // Temporary hack since I don't want to declare all members, yet | ||
| [key: string]: any; |
There was a problem hiding this comment.
Which members are not declared?
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:
Error. I usedObject.setPrototypeOfto get around it. See https://github.com/Microsoft/TypeScript-wiki/blob/master/Breaking-Changes.md#extending-built-ins-like-error-array-and-map-may-no-longer-work for a problem description.tsc, the generated UMD module does not include a global fallback. See umd module compiler option doesn't have a fallback for global namespace. microsoft/TypeScript#8436. To work around that, I used rollup.js as a bundler, however this seems a bit heavy-weightrun-javascriptscript inside thetestsrepository does not actually build the JavaScript runtime but only links itI 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
readBitsIntwas previously set to the exact same functioninstance as
readBitsIntBE. This is not possible in TypeScript.Instead, this
readBitsIntis now implemented as a separate functionwhich 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-javascriptscript inside thetestsrepository does not actually build the JavaScript runtime butonly links it