Repository navigation
TypeScript Core - #8861
TypeScript Core#8861chharvey wants to merge 38 commits into
Conversation
|
TODO:
|
|
In general this looks pretty good to me, but not being a TypeScript person, I'm not the right reviewer here. Hopefully @ericvergnaud or @brendandahl or other people already cc'd on this issue have time and interest to review this. |
|
Hi @kripken @tlively @MaxGraey @ericvergnaud , wondering if you could give this a review and/or more feedback. Happy to answer any questions. I also have a stacked PR (#8881 is next, but there will be more) that depend on this, to break up #8826 into smaller parts as suggested. Is there continued group interest in moving forward with this? If there is some hesitancy maybe we can continue the discussion in #8656. Thanks! |
|
I do think it would be nice to move forward with a TS API, especially given how much LLMs love to write typescript. I'm in the same boat as @kripken where I don't really have the background to do a useful review, though. We'll talk with @brendandahl and try to come up with a way forward soon. Thanks for your work on this and your patience in the meantime! |
|
Given what's happened re TS over the past 2 years i.e. nothing, I believe the governance issue remains unsolved. |
brendandahl
left a comment
brendandahl
left a comment
There was a problem hiding this comment.
Overall, looks pretty good. I'd like to get the typechecking test issue addressed though. Also, I may be missing something, but it also seems we're missing an end-to-end test that uses the result of npm compile in a TypeScript project. This doesn't need to be done in this PR (already huge), but would be good to add in the future.
The one thing that has changed, is the burden of maintaining this (due to LLMs). I think we should try out merging this PR and see how keeping it up to date goes. As Thomas mentioned, I don't think this will be too bad anymore since |
|
@brendandahl That sounds great, thanks! @ericvergnaud Your review of this code would also be appreciated, if you have interest. I think we are in a better place now than before. |
|
One other issue I noticed, after building locally I had a lot of extra untracked files. I'm guessing there's some gitignore that needs to be updated. |
|
@brendandahl Which files did you notice? I updated the root |
…nto merge/oscar/expose-new-exceptions
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a TypeScript port of the Binaryen API, adding configuration files, documentation, package metadata, and core TypeScript source files. The review feedback primarily focuses on a performance concern where several enums (such as Feature, ExpressionRunnerFlag, and Type) are initialized via synchronous WebAssembly function calls at module load time, which introduces unnecessary startup overhead. It is recommended to define these as standard TypeScript enums with literal constant values. Additionally, there is a mismatch between the Node.js engine version specified in package.json and package-lock.json that should be aligned.
| export enum Feature { | ||
| MVP = BinaryenObj["_BinaryenFeatureMVP"](), | ||
| Atomics = BinaryenObj["_BinaryenFeatureAtomics"](), | ||
| MutableGlobals = BinaryenObj["_BinaryenFeatureMutableGlobals"](), | ||
| NontrappingFPToInt = BinaryenObj["_BinaryenFeatureNontrappingFPToInt"](), | ||
| SIMD128 = BinaryenObj["_BinaryenFeatureSIMD128"](), | ||
| BulkMemory = BinaryenObj["_BinaryenFeatureBulkMemory"](), | ||
| SignExt = BinaryenObj["_BinaryenFeatureSignExt"](), | ||
| ExceptionHandling = BinaryenObj["_BinaryenFeatureExceptionHandling"](), | ||
| TailCall = BinaryenObj["_BinaryenFeatureTailCall"](), | ||
| ReferenceTypes = BinaryenObj["_BinaryenFeatureReferenceTypes"](), | ||
| Multivalue = BinaryenObj["_BinaryenFeatureMultivalue"](), | ||
| GC = BinaryenObj["_BinaryenFeatureGC"](), | ||
| Memory64 = BinaryenObj["_BinaryenFeatureMemory64"](), | ||
| RelaxedSIMD = BinaryenObj["_BinaryenFeatureRelaxedSIMD"](), | ||
| ExtendedConst = BinaryenObj["_BinaryenFeatureExtendedConst"](), | ||
| Strings = BinaryenObj["_BinaryenFeatureStrings"](), | ||
| MultiMemory = BinaryenObj["_BinaryenFeatureMultiMemory"](), | ||
| StackSwitching = BinaryenObj["_BinaryenFeatureStackSwitching"](), | ||
| SharedEverything = BinaryenObj["_BinaryenFeatureSharedEverything"](), | ||
| FP16 = BinaryenObj["_BinaryenFeatureFP16"](), | ||
| BulkMemoryOpt = BinaryenObj["_BinaryenFeatureBulkMemoryOpt"](), | ||
| CallIndirectOverlong = BinaryenObj["_BinaryenFeatureCallIndirectOverlong"](), | ||
| // TODO: CustomDescriptors | ||
| AcquireReleaseAtomics = BinaryenObj["_BinaryenFeatureAcquireReleaseAtomics"](), | ||
| CustomPageSizes = BinaryenObj["_BinaryenFeatureCustomPageSizes"](), | ||
| // TODO: Multibyte | ||
| WideArithmetic = BinaryenObj["_BinaryenFeatureWideArithmetic"](), | ||
| CompactImports = BinaryenObj["_BinaryenFeatureCompactImports"](), | ||
| RelaxedAtomics = BinaryenObj["_BinaryenFeatureRelaxedAtomics"](), | ||
| All = BinaryenObj["_BinaryenFeatureAll"](), | ||
| } |
There was a problem hiding this comment.
Initializing the Feature enum by calling WebAssembly functions at module load time introduces unnecessary startup overhead. Since these values are stable ABI constants, they should be defined as standard TypeScript enums with literal/constant values (e.g., MVP = 0, Atomics = 1 << 0, etc.). This avoids runtime overhead, improves startup performance, and allows TypeScript to treat them as constant/literal enums for better type safety and inlining.
export enum Feature {
MVP = 0,
Atomics = 1 << 0,
MutableGlobals = 1 << 1,
NontrappingFPToInt = 1 << 2,
SIMD128 = 1 << 3,
BulkMemory = 1 << 4,
SignExt = 1 << 5,
ExceptionHandling = 1 << 6,
TailCall = 1 << 7,
ReferenceTypes = 1 << 8,
Multivalue = 1 << 9,
GC = 1 << 10,
Memory64 = 1 << 11,
RelaxedSIMD = 1 << 12,
ExtendedConst = 1 << 13,
Strings = 1 << 14,
MultiMemory = 1 << 15,
StackSwitching = 1 << 16,
SharedEverything = 1 << 17,
FP16 = 1 << 18,
BulkMemoryOpt = 1 << 19,
CallIndirectOverlong = 1 << 20,
// TODO: CustomDescriptors
AcquireReleaseAtomics = 1 << 22,
CustomPageSizes = 1 << 23,
// TODO: Multibyte
WideArithmetic = 1 << 25,
CompactImports = 1 << 26,
RelaxedAtomics = 1 << 27,
All = (1 << 28) - 1,
}There was a problem hiding this comment.
the TypeScript layer is just supposed to be a direct interface to the Binaryen API. It might be more performant to write the literals as suggested, but that would mean falling out of sync with the C source. Keeping it as BinaryenObj["_BinaryenFeature..."]() ensures a single source of truth.
| export enum Type { | ||
| // ### Binaryen-Only Types ### // | ||
| /** Type with stack effect `[t*] -> [t*]`. @category Binaryen-Only Types */ | ||
| unreachable = BinaryenObj["_BinaryenTypeUnreachable"](), | ||
| /** Type with stack effect `[t*] -> []`. Not to be confused with the WASM heap type `none`. @category Binaryen-Only Types */ | ||
| none = BinaryenObj["_BinaryenTypeNone"](), | ||
| /** Used only for auto-detecting block types. @category Binaryen-Only Types */ | ||
| auto = BinaryenObj["_BinaryenTypeAuto"](), | ||
|
|
||
| // ### Number & Vector Types ### // | ||
| /** 32-bit integer. @category Number & Vector Types */ | ||
| i32 = BinaryenObj["_BinaryenTypeInt32"](), | ||
| /** 64-bit integer. @category Number & Vector Types */ | ||
| i64 = BinaryenObj["_BinaryenTypeInt64"](), | ||
| /** 32-bit float. @category Number & Vector Types */ | ||
| f32 = BinaryenObj["_BinaryenTypeFloat32"](), | ||
| /** 64-bit float. @category Number & Vector Types */ | ||
| f64 = BinaryenObj["_BinaryenTypeFloat64"](), | ||
| /** 128-bit vector (SIMD). @category Number & Vector Types */ | ||
| v128 = BinaryenObj["_BinaryenTypeVec128"](), | ||
|
|
||
| // ### Reference Types ### // | ||
| /** `(ref null any)` @category Reference Types */ | ||
| anyref = BinaryenObj["_BinaryenTypeAnyref"](), | ||
| /** `(ref null eq)` @category Reference Types */ | ||
| eqref = BinaryenObj["_BinaryenTypeEqref"](), | ||
| /** `(ref null i31)` @category Reference Types */ | ||
| i31ref = BinaryenObj["_BinaryenTypeI31ref"](), | ||
| /** `(ref null struct)` @category Reference Types */ | ||
| structref = BinaryenObj["_BinaryenTypeStructref"](), | ||
| /** `(ref null array)` @category Reference Types */ | ||
| arrayref = BinaryenObj["_BinaryenTypeArrayref"](), | ||
| /** `(ref null string)` @category Reference Types */ | ||
| stringref = BinaryenObj["_BinaryenTypeStringref"](), | ||
| /** `(ref null none)` @category Reference Types */ | ||
| nullref = BinaryenObj["_BinaryenTypeNullref"](), | ||
| /** `(ref null func)` @category Reference Types */ | ||
| funcref = BinaryenObj["_BinaryenTypeFuncref"](), | ||
| /** `(ref null nofunc)` @category Reference Types */ | ||
| nullfuncref = BinaryenObj["_BinaryenTypeNullFuncref"](), | ||
| /** `(ref null exn)` @category Reference Types */ | ||
| exnref = BinaryenObj["_BinaryenTypeExnref"](), | ||
| /** `(ref null noexn)` @category Reference Types */ | ||
| nullexnref = BinaryenObj["_BinaryenTypeNullExnref"](), | ||
| /** `(ref null extern)` @category Reference Types */ | ||
| externref = BinaryenObj["_BinaryenTypeExternref"](), | ||
| /** `(ref null noextern)` @category Reference Types */ | ||
| nullexternref = BinaryenObj["_BinaryenTypeNullExternref"](), | ||
| } |
There was a problem hiding this comment.
Initializing the Type enum (and other enums in this file like HeapType, PackedType, ExpressionId, etc.) by calling WebAssembly functions at module load time introduces significant startup overhead (hundreds of synchronous WASM calls on import).
Since these values are stable ABI constants defined by the WebAssembly specification and the Binaryen C API, they should be defined as standard TypeScript enums with literal/constant values (e.g., none = 0, unreachable = 1, i32 = 2, etc.).
This would:
- Eliminate 300+ WASM function calls at startup, significantly improving startup performance.
- Allow the enums to be fully inlined by the TypeScript compiler (or be standard literal enums), which are much more useful for type checking and type safety.
- Keep the JS module synchronous and lightweight to load.
There was a problem hiding this comment.
the current JS file (binaryen-js.post.js) already does this; this PR just converts to TS. if this is still a concern then it should be brought up as a separate discussion/PR
The first of several PRs in an attempt to break up #8826 into smaller parts. This PR sets up core typescript functionality and ports over only the top-level types, constants, and enums of the JS API.
Changes:
ts/folder with README.md, package.json, tsconfig.json, and other project filesCMakeLists.txt, exporting needed variables & methods (likeHEAP8) from Emscripten into the JS buildts/src/-pre.tsimports the Emscripten-builtBinaryenfunction, calls and awaits it, and exports that as an internal object calledBinaryenObj(AssemblyScript calls this objectbinaryenand exports that publicly).ts/src/**: the typescript library sits on top of the Emscripten artifact. it accesses the WASM bindings onBinaryenObjimported from-pre.ts.ts/src/binaryen.tsfor users, exporting all the parts of the public-facing API. Users use a namespace import instead of a default import:users will still use the same API (
binaryen.ExpressionRef,binaryen.i32,binaryen.Module, etc.)ts/tests/. hopefully this will take some of the weight off the python testsNon-Changes:
API Deprecations (see
ts/src/-deprecations.tsfor full list; will evolve with more PRs):binaryen.Features→binaryen.Feature)binaryen.Features, but there is a doc-comment@deprecatedwarning for intellisense support)Get started:
See
ts/package.json,ts/README.md, andts/docs/API-Overview.md(in this PR) for details.