Port JavaScript runtime to TypeScript #25
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
|
Looking forward to this PR. 👀 |
generalmimon
added a commit
to kaitai-io/kaitai_struct_compiler
that referenced
this pull request
Sep 28, 2024
Until now, byte array literals translated for JavaScript were of type `number[]`, as TypeScript would call this type (in pure JS terminology, it's an `Array` object with numbers). For example, a value instance defined as `instances/v/value: '[65, 126]'` would be translated as `this._m_v = [65, 126];` in the generated JS code. However, byte arrays created in any other way would use the [`Uint8Array`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8Array) type, which is the standard type for byte arrays in JavaScript. Namely any byte array parsed from stream would be a `Uint8Array` object and also a *non-literal* byte array like `[123, 246 + 0].as<bytes>` would be generated as `new Uint8Array([123, 246 + 0])`. I believe that using a different type for byte array literals than for byte arrays created in any other way was never intentional. It seems we simply forgot to override `doByteArrayLiteral` in `JavaScriptTranslator` (but `doByteArrayNonLiteral` is overridden correctly, which is weird), so we ended up using the default `BaseTranslator.doByteArrayLiteral` implementation, which produces the `[65, 126]` syntax. JavaScript is dynamically typed, so you can treat `number[]` the same as `Uint8Array` in most cases (e.g. indexing will work the same), but this discrepancy now causes problems when porting our JavaScript runtime library to TypeScript (see kaitai-io/kaitai_struct_javascript_runtime#25), because you would have to annotate a lot of parameter types as `number[] | Uint8Array` (which doesn't really make sense). Moreover, I discovered that generating byte array literals as `number[]` creates a bug in this case: ```ksy meta: id: bytes_to_s instances: literal: value: '[65, 126].to_s("UTF-8")' ``` Before this commit, `[65, 126].to_s("UTF-8")` would be translated as `KaitaiStream.bytesToStr([65, 126], "UTF-8")` for JavaScript. But our `bytesToStr` implementation won't actually accept `number[]` for non-ASCII encodings. See [`KaitaiStream.js:650-657`](https://github.com/kaitai-io/kaitai_struct_javascript_runtime/blob/2acb0d8b09fde9c95fc77ee3019f6d6b1c73f040/KaitaiStream.js#L650-L657): ```js KaitaiStream.bytesToStr = function(arr, encoding) { if (encoding == null || encoding.toLowerCase() === "ascii") { return KaitaiStream.createStringFromArray(arr); } else { if (typeof TextDecoder === 'function') { // we're in the browser that supports TextDecoder return (new TextDecoder(encoding)).decode(arr); } else { ``` If `TextDecoder` is available (which is the case in a browser or since Node.js 11), the error when trying to access the `literal` instance above will look like this in the browser (Google Chrome 129): ``` TypeError: Failed to execute 'decode' on 'TextDecoder': parameter 1 is not of type 'ArrayBuffer'. at KaitaiStream.bytesToStr (https://ide.kaitai.io/devel/lib/_npm/kaitai-struct/KaitaiStream.js:656:42) at BytesToS.get (eval at initCode (https://ide.kaitai.io/devel/js/v1/kaitaiWorker.js:156:9), <anonymous>:27:38) at fetchInstance (https://ide.kaitai.io/devel/js/v1/kaitaiWorker.js:123:20) ... ``` ... and like this in Node.js 12: ``` TypeError [ERR_INVALID_ARG_TYPE]: The "input" argument must be an instance of ArrayBuffer or ArrayBufferView. Received an instance of Array at TextDecoder.decode (internal/encoding.js:412:15) at Function.KaitaiStream.bytesToStr (/runtime/javascript/KaitaiStream.js:656:42) at BytesToS.get (compiled/javascript/BytesToS.js:26:38) at /tests/spec/javascript/test_bytes_to_s.js:8:24 ... ``` So using `Uint8Array` for all byte arrays will not only be more consistent, but will also fix this bug.
This is now possible thanks to kaitai-io/kaitai_struct_compiler@ec064e3
This fixes the `name` property of class constructors. For example, `KaitaiStream.ValidationNotEqualError.name` now returns `'ValidationNotEqualError'`. This is consistent with how the built-in error types work - for example, `RangeError.name` is `'RangeError'`, as you would expect. Previously, when the classes were anonymous, `KaitaiStream.ValidationNotEqualError.name` returned `'class_4'` (which was an automatically generated name that the TypeScript compiler used in the output ES5 code). It's worth noting that error constructors were anonymous also in our handwritten `KaitaiStream.js` before we migrated to TypeScript ( `KaitaiStream.ValidationNotEqualError.name` used to be the empty string `''`), so keeping them anonymous wouldn't really be a regression, but it's nice to improve it now. I noticed this issue because we use [`assert.throws`](https://nodejs.org/docs/latest-v20.x/api/assert.html#assertthrowsfn-error-message) in KST with arguments as in the "Validate instanceof using constructor:" example. If this assertion fails, Node.js includes the `name` property of the constructor in the error message if it's not empty. As a result, the error message was the following when a `ValidationNotEqualError` was expected but not thrown: ``` Missing expected exception (class_4). AssertionError [ERR_ASSERTION]: Missing expected exception (class_4). at /tests/helpers/javascript/testHelperThrows.js:12:16 at FSReqCallback.readFileAfterClose [as oncomplete] (node:internal/fs/read/context:68:3) ``` Now it is `Missing expected exception (ValidationNotEqualError)`.
The status of `prepublish` scripts is still unclear and confusing to me. https://docs.npmjs.com/cli/v10/using-npm/scripts#life-cycle-scripts states "prepublish (DEPRECATED)", then https://docs.npmjs.com/cli/v10/using-npm/scripts#prepare-and-prepublish explains how `prepublish` is a confusing name (because it doesn't actually run with `publish` at all), but then it still recommends "use a `prepublish` script" for tasks such as "Compiling CoffeeScript source code into JavaScript". That doesn't sound like it's deprecated to me if it's still recommended in some situations, so maybe the documentation is wrong. I suppose the least surprising option is not to rely on any automatic "hook" and always require running `npm run build` manually.
generalmimon
approved these changes
Oct 2, 2024
generalmimon
added a commit
to kaitai-io/kaitai_struct_docker_images
that referenced
this pull request
Oct 2, 2024
generalmimon
added a commit
to kaitai-io/kaitai_struct_tests
that referenced
this pull request
Oct 2, 2024
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a continuation of #18, resolving some of the comments there and adding additional types and fixes.
Notable changes from #18:
mastersuper()callKaitaiStream.jsfile and added it to the.gitignore.npmignoreSome caveats from that PR still apply:
Object.setPrototypeOfto work around potential Error inheritence issuesrollup.jsas the bundler to get a global fallback for umd modulesAlong with some additional notes:
arr.lengthandarr[0]I've typed asArrayLike<number>instead of something more specific likeUint8Array)_trimAlloc()) are intended to be private and left the rest public. It's possible there's other methods that were originally intended to be private but weren't marked.any, since it was hard to find examples of them being used and theoretically anything could be passed in.I've managed to run the kaitai tests with this in a node environment, and the results match the current JS version.
I don't feel 100% confident with this yet - but that's mostly because it wasn't clear what all the types should be. The functionality itself hasn't changed much and should still be solid.