Add TypeScript typings.

jchv commented 5 years ago

Took a few tries but I think I got it right. It should work when used as an ambient module or through a module system.

I think it would be nice to go further and convert the whole dang thing to TypeScript. It would even catch some errors, including an operator precedence one in the signed integer handling (I'll create another PR for it.) So I think it's definitely worth the trouble to have another build step to build the usual ES5 UMD module. There's a couple issues though:

Moving everything to TypeScript would add a dependency (at minimum we need the Node.JS typings for Buffer) and a build step. Today, this module is really simple. It has no dependencies and no build steps at all. I think that is really nice, and therefore getting rid of that property requires more consideration.
In ES6 classes, you can extend the Error class normally, but if you compile this down to ES5 prototype inheritance, it gets a little broken. There's hacks to work around it (like Object.setPrototypeOf) but I definitely think this sucks. Compiling to ES6 classes would be doable, but it would break compatibility with old browsers. The setPrototypeOf hack would break compatibility with IE <10. The existing approach checks most boxes but it can't be expressed in TypeScript. :slightly_frowning_face:

GreyCat commented 5 years ago

My only concern right now is that this is not being tested in any way. Can we add some sort of CI step to ensure that these .d.ts files at least compile?

koczkatamas commented 5 years ago

I will be able to review this tonight hopefully.

jchv commented 5 years ago

@GreyCat Yeah, it would be nice to test it. However, at that point I'd have to add a dev dependency on at least the TypeScript compiler. Is it worth just porting the whole thing over at that point? The caveat regarding extending errors above is the only other thing that stopped me from proposing that.

GreyCat commented 5 years ago

I totally won't mind moving to TypeScript for everything, as long as we still support older (ES3?) targets.

That said, though, currently, we don't have any tests for in-browser JS engines, and this is probably something to consider here. Mocha, which we use, technically can be used to launch an in-browser test, and we can even automate that to some extent by running this in headless chrome or something similar, but it will be a major hurdle to jump over to make it run in CI with really old browsers (IE6?).

So, on that trrack, probably we should start at deciding on that minimal baseline and adding relevant tests. Until then, I'm a huge proponent of "does not break tests = valid".

jchv commented 5 years ago

SGTM. In that light, maybe the right direction is incremental steps. Adding browser testing before considering porting to TypeScript will save us some pain.

I do believe that TypeScript's ES5 output will be compatible with any browser that has native typed arrays (IE10+) and then some. So as far as compatibility goes, I think tsc transpilation is sufficient, if we establish our baseline as native typed array support. It seems reasonable, since performance of emulated typed arrays is pretty bad.

GreyCat commented 5 years ago

native typed array support

Probably a solid idea: https://caniuse.com/#feat=typedarrays

That actually means that we'll be officially supporting IE11+, Edge, FF 4+, Chrome 7+, Opera 12+. Everything else is probably more or less safe to ignore.

Now I actually wonder if there are any ready-made browser testing farms that can be used to export test unit results from something similar to Mocha, so we could leverage it to run in-browser tests?

GreyCat commented 5 years ago

Added main project issue: https://github.com/kaitai-io/kaitai_struct/issues/540 to track in-browser CI idea.

k2on commented 3 years ago

Is this getting merged?

kaitai-io / kaitai_struct_javascript_runtime

Add TypeScript typings. #10