Generate typescript

[ericvergnaud]

Hi, following-up on issue #6145 and PR #6175

This is a first iteration, that already covers all features of index.d.ts in binaryen.ts repo, plus a few additions: TypeBuilder and minimal arrays (WIP).

The purpose of this PR is to ensure that the proposed approach is fine, notably because it is a bit different from the one originally envisaged i.e. produce the binaryen.js-post.js file from a ts file. I tried that approach and it failed, so submitting a new approach.

Why did the original approach fail ?

the generated js file contains a single function (Binaryen) that needs to be invoked before accessing wasm entry points. This is fine but it makes it impossible to specify consts and enums in the ts code itself. Rather, they need to be populated once the code is loaded (in binaryen_wasm, this is done by the initializeConstants method, called after the module is loaded). Overriding consts and enums in Typescript is not supported.
in the existing implementation, the Module 'class' wraps the entire module by adding methods to it, rather than using it from a separated class. This works in JS (and a manually built .d.ts file) but is impossible to mimic when generating JS from Typescript.
some components of the tool chain fail with the JS generated from Typescript.
Overall, working around all these issues would have led to breaking changes and extremely hacky code.

What is the proposed approach ?

Following feedback from PR #6175 the proposed approach in this PR is to ensure a single source of truth for the JS/TS APIs, without creating a new target, at the cost of minor breaking changes:

1. the exported Binaryen is now already awaited, so a consumer no longer has to do that.
2. each target now produces 4 files instead of 1, as below:

There are also a number of small breaking changes that can be reverted, which will be done as part of this PR if required ( if the PR is accepted):

4. xxxID constants have been dropped, purely out of lazyness <- I can't think of a scenario where one would need them
5. a few entry points have been moved or renamed for consistency

The file generated by emscripten is smaller, simply because the post_js file is reduced to the bare minimum.
The build generates additional files:

• .js: wraps the file generated by emscripten, and enriches it with a JS API
• .d.ts: type declarations for the above
• .d.ts.map: source map for the above

The produced wrapper files also need to be minified, that's WIP.

WIth this design, it becomes easier to wrap binaryen entry points, because they can be declared in a simple manner.
Also, the generated files can be used directly without packaging, which is pretty useful for testing. I have migrated my code base to use it and haven't found any issues yet. It also rationalizes the API a bit, by grouping and renaming some wrapper functions for improved clarity.

The GC related stuff (TypeBuilder and 'arrays' section) is obviously incomplete, this part is WIP and will be enhanced through future PRs (if this one is accepted).

[ericvergnaud]

@tlively @kripken happy new year! It'd be great to get your feedback on this

[ericvergnaud]

@tlively @kripken sorry to sound a bit pushy... any chance we can progress this ?

[kripken]

My difficulty in reviewing this PR is that I am not that familiar with TS or actual usage of binaryen.js's JS API. So I am not sure if the minor breaking changes here will be a difficulty for users or not.

Perhaps open an issue for discussion on the buildbot repo https://github.com/AssemblyScript/binaryen.js/ ? If everyone there is happy with this approach that would make me a lot more confident here.

[ericvergnaud]

My difficulty in reviewing this PR is that I am not that familiar with TS or actual usage of binaryen.js's JS API. So I am not sure if the minor breaking changes here will be a difficulty for users or not.

Perhaps open an issue for discussion on the buildbot repo https://github.com/AssemblyScript/binaryen.js/ ? If everyone there is happy with this approach that would make me a lot more confident here.

Done

[jayphelps]

I'm fine with the proposed breaking changes on my end.

[phated]

@kripken I think Binaryen should actually move the opposite direction than this PR. I've been working on a change that would allow the "post" wrapper to be removed and be implemented completely separately.

For Grain, we've found the JS wrapper doesn't keep feature parity but the changes required for dropping the JS wrapper would allow us to implement everything via the low-level APIs.

I believe my change would also solve the contention that this PR is trying to solve (in a different way) because the wrapper can be written in TypeScript in the upstream repository while consuming the low-level APIs.

[ericvergnaud]

@kripken I think Binaryen should actually move the opposite direction than this PR. I've been working on a change that would allow the "post" wrapper to be removed and be implemented completely separately.

For Grain, we've found the JS wrapper doesn't keep feature parity but the changes required for dropping the JS wrapper would allow us to implement everything via the low-level APIs.

I believe my change would also solve the contention that this PR is trying to solve (in a different way) because the wrapper can be written in TypeScript in the upstream repository while consuming the low-level APIs.

@phated interesting.
You might want to look more into this PR. It actually removes almost ALL 'post' wrapper code to a separate file, such that the files generated by emscripten only contain low-level APIs (while the high-level APIs now live in separate output files).
So there's a genuine possibility that this PR actually goes in the direction you need, by separating concerns.
(to be fair, the above only applies to JS code from Binaryen, not to JS code from Emscripten. I haven't yet looked at dropping all the FS stuff, which makes sense when coming from C, but maybe not so much when using Binaryen's IR for non-LLVM languages...)

See src/ts/binaryen_post.ts

[CountBleck]

I'll pitch in my two cents.

It might make sense to use JSDoc annotations in the existing binaryen.js-post.js file, and make any changes necessary for TypeScript to infer types for unannotated parts. That way, perhaps the binaryen.js buildbot could use tsc to generate a .d.ts file from the JSDoc types.

I can't say if that's more work than rewriting the whole thing in TypeScript, but it's an alternative.

Also, Windows doesn't have sed if I remember correctly.

[ericvergnaud]

I'll pitch in my two cents.

It might make sense to use JSDoc annotations in the existing binaryen.js-post.js file, and make any changes necessary for TypeScript to infer types for unannotated parts. That way, perhaps the binaryen.js buildbot could use tsc to generate a .d.ts file from the JSDoc types.

I can't say if that's more work than rewriting the whole thing in TypeScript, but it's an alternative.

Also, Windows doesn't have sed if I remember correctly.

@CountBleck Thanks. However the work of 'rewriting the whole thing in TypeScript' is already done, so not sure what benefits we would get from redoing it using JSDoc ?

I'll look at sed alternatives for windows.

[ericvergnaud]

I'll look at sed alternatives for windows.

Tentatively fixed, not tested. @kripken Is it possible to let the workflow run without merging ?

[ericvergnaud]

If you look at src/ts/binaryen_post.ts in #6192 you'll find that it's down to almost nothing, so Grain could maybe adopt the now quite lean generated wasm, and build their own wrapper as desired ?

[ospencer]

Thanks @ericvergnaud for your work here—there have certainly been some pain points around the high-level JS API.

I think this repository's responsibility is just the core C++ implementation. We should have the wasm and JS targets, but the high-level bindings for languages other than C/C++ should ideally live in other projects, like they do for binaryen.ml.

I agree with you that it's not ideal that the binaryen.js repo isn't under the WebAssembly org, especially given its broad use, but perhaps if the AS team is willing it can be transferred.

A combination of #6224 along with merging this PR into binaryen.js instead makes the most sense to me. The packaging and documentation live there and this high-level TypeScript wrapper code should too.

[ericvergnaud]

This PR is raising governance questions which are discussed in #6225

[kripken]

A note from my side, I am not a user of the JS bindings so I don't have a preference between this and #6224. Whichever path users of binaryen.js prefer sounds good to me.

One note, though, about @ospencer 's comment:

I think this repository's responsibility is just the core C++ implementation. We should have the wasm and JS targets, but the high-level bindings for languages other than C/C++ should ideally live in other projects, like they do for binaryen.ml.

I mostly agree with that, but there is a possible reason it would make sense to keep them here: if we automate them. Most of the bindings are about Expression operations, and we do have the machinery to operate on those in an abstract way (using wasm-delegations-fields.def). Using that, we could write a little code that autogenerates all the Expression operations (creating the different types, inspecting them, modifying them, etc.). That could save a lot of work in the long term. I haven't had time to look into that myself, though I've hoped to.

But again, I do agree with the point that language-specific bindings generally make sense in separate repos where they can focus on just that, and they can be fully tested and so forth. Perhaps one way forward is to aim to autogenerate the C bindings in this repo, and leave other language bindings to other repos. However, I think those other languages could also benefit from that automation, which I'm not sure would be easy to do outside this repo, but I'm not sure.

[ericvergnaud]

I could use some help to fix the failing build.
The failure is caused by not finding "/home/runner/work/binaryen/binaryen/out/test/binaryen_raw_wasm.js" but that file gets build to "binaryen/dist" so not sure what needs to be adjusted to fix the issue. Do I need to somehow copy the generated file as part of the test script ?

[ericvergnaud]

I'll fix emscripten build (directory issue) if we move forward with this, but I might need help for mingw...

[samestep]

@kripken since #6225 seems currently undecided, would it be OK if I take just the TypeBuilder stuff from this PR and put it in a separate PR to hopefully merge sooner?

[kripken]

@samestep That sounds reasonable.