-
Notifications
You must be signed in to change notification settings - Fork 810
Generate typescript #6175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Generate typescript #6175
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
e76aa6e
progress
ericvergnaud 16cae25
has complete i32, i64, f32 and f64
ericvergnaud 19378e6
document API changes
ericvergnaud b1be833
generates v128
ericvergnaud 76846ff
support i8x16
ericvergnaud 96add7c
support i16x8
ericvergnaud b69507a
support i32x4
ericvergnaud 6d99060
support i64x2
ericvergnaud 6e6d209
support f32x4
ericvergnaud f5866e6
support f64x2
ericvergnaud 255b56f
more stuff
ericvergnaud cc49c4c
more stuff
ericvergnaud 3a2b2a2
progress
ericvergnaud a52dfe3
support ExpressionInfo
ericvergnaud 4b4460e
migration of index.d.ts from binaryen.js complete
ericvergnaud 0520e32
reinstate deprecated methods
ericvergnaud e370f1c
fix return types
ericvergnaud 839b665
a base version i.e. symbols are exported, but not tested yet
ericvergnaud 76a0e7c
Works for all existing tests in my project
ericvergnaud ec80d01
rename for consistency
ericvergnaud 898cb2d
Fix typos
ericvergnaud 36c0747
Fix Feature API
ericvergnaud File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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 file contains hidden or 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 file contains hidden or 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
declare var Module: object; | ||
declare var out: (s: string) => void; | ||
declare var stringToAscii: (s: string, ptr: number) => void; | ||
declare var stackSave: () => number; | ||
declare var stackAlloc: (size: number) => number; | ||
declare var stackRestore: (ref: number) => void; | ||
declare var allocateUTF8OnStack: (s: string) => number; | ||
declare var _BinaryenSizeofLiteral: () => number; | ||
declare var _BinaryenSizeofAllocateAndWriteResult: () => number; | ||
declare var UTF8ToString: (ptr: number) => string | null; | ||
|
||
type Writer = (s: string) => void; | ||
function swapOut(writer: Writer): Writer { | ||
const saved = out; | ||
out = writer; | ||
return saved; | ||
} | ||
|
||
Module['utils'] = { | ||
"swapOut": swapOut, | ||
"stringToAscii": stringToAscii, | ||
"stackSave": stackSave, | ||
"stackAlloc": stackAlloc, | ||
"stackRestore": stackRestore, | ||
"allocateUTF8OnStack": allocateUTF8OnStack, | ||
"_BinaryenSizeofLiteral": _BinaryenSizeofLiteral, | ||
"_BinaryenSizeofAllocateAndWriteResult": _BinaryenSizeofAllocateAndWriteResult, | ||
"UTF8ToString": UTF8ToString | ||
}; |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to avoid having to repeat all these flags from the vanilla binaryen_wasm build? Do we need another target at all? Can we unconditionally emit the TS wrapper for all binaryen_js and binaryen_wasm builds instead?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An option I haven't considered is to apply for JS the same approach than TS i.e. rather than embed the JS code in the generated JS, have it live aside and consume the wasm. That way we would have a single wasm target generating:
That's a breaking change in that the JS now requires 2 files instead of 1, but I don't anticipate anyone complaining about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually tried out this approach as an experiment, and technically speaking it would work. Let me know if that's a better idea and I'll evolve this PR accordingly.
That said it not only impacts the binaryen_wasm.js target, but also the binaryen_js.js one, since they share the binaryen.js-post.js post-generation file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kripken, what do you think about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding adding another file here, that seems possible in general but we do build for node.js using
-sSINGLE_FILE
. I'm actually not sure why we do that, but if we want to add another file here we should probably look into removingSINGLE_FILE
first.Separately, could another option be to always build for TS and let users use a TS-to-JS transpiler if they don't want TS? How is this kind of thing usually done in the JS/TS ecosystem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I'm keeping the current JS is backwards compatibility. The build script already creates a JS from TS, but it's a bit different from the current one. If binaryen_wasm only has a few consumers then it's obviously simpler to drop the current one and advertise the breaking changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it's done the usual way I.e. run tsc, which generates the JS file, a d.ts file and a .map file for debugging. Pretty standard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks.
When you say "a bit different" is that stuff like code size or does it also behave differently? Is it a drop-in replacement?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new file behaves differently:
I can't change 1, but I can fix 2 and 3 if breaking change 1 is acceptable and that leads do a single source of truth and just 2 targets (like we have today) instead of 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried turning off SINGLE_FILE. The purpose of this flag seems to be the following:
(that doesn't affect the async loading requirement which is the blocking point for typescript, because you can't declare const and enums at runtime)