Conversation
|
For maintainers only:
|
| }, | ||
| { | ||
| test: /\.wast$/i, | ||
| loader: "@webassemblyjs/wast-loader", |
There was a problem hiding this comment.
I don't know if this is wanted but I think it's a common use case to enable it by default?
There was a problem hiding this comment.
Using loaders by default comes with a lot of problems. I would keep it like other loaders.
There was a problem hiding this comment.
For the test suite move this into test/TestCases.template.js
| "@webassemblyjs/ast": "1.4.3", | ||
| "@webassemblyjs/wasm-edit": "1.4.3", | ||
| "@webassemblyjs/wasm-parser": "1.4.3", | ||
| "@webassemblyjs/wast-loader": "^1.5.4", |
There was a problem hiding this comment.
To avoid duplicated deps (with other versions), we should align these, but it's already done by some PR i think.
There was a problem hiding this comment.
Should be in devDependencies instead
There was a problem hiding this comment.
Yes, if we move it from the default options
ooflorent
left a comment
There was a problem hiding this comment.
I like the idea of supporting wast by default. This change looks good.
|
btw I have the |
| type: "webassembly/experimental" | ||
| }, | ||
| { | ||
| test: /\.wast$/i, |
There was a problem hiding this comment.
The recommended file extension for WebAssembly code in textual format is .wat.
There was a problem hiding this comment.
Actually wast (WebAssembly script format) and wat (text format) are two different syntaxes but i agree it's better to switch to .wat.
| }, | ||
| { | ||
| test: /\.wast$/i, | ||
| loader: "@webassemblyjs/wast-loader", |
There was a problem hiding this comment.
Using loaders by default comes with a lot of problems. I would keep it like other loaders.
| @@ -1,13 +1,13 @@ | |||
| it("should support wasm compiled from c++", function() { | |||
| return import("./memory3.wasm").then(function(wasm) { | |||
| return import("./memory3.wast").then(function(wasm) { | |||
There was a problem hiding this comment.
I would keep the decoding tests in wasm as they try to test special encodings of the wasm binary (like paddings). This is not possible with wast (wat)
There was a problem hiding this comment.
The other tests can be changed to .wat for better readability.
| "@webassemblyjs/ast": "1.4.3", | ||
| "@webassemblyjs/wasm-edit": "1.4.3", | ||
| "@webassemblyjs/wasm-parser": "1.4.3", | ||
| "@webassemblyjs/wast-loader": "^1.5.4", |
There was a problem hiding this comment.
Should be in devDependencies instead
| }, | ||
| { | ||
| test: /\.wast$/i, | ||
| loader: "@webassemblyjs/wast-loader", |
There was a problem hiding this comment.
For the test suite move this into test/TestCases.template.js
|
I kept the LEB128 padded one to ensure we are parsing them, everything else has been changed to wast. |
|
As a side note, wabt's WASM to WAT adds a bit of latency, the test is probably failing because of that. Should we increase the timeout? In the future we'll be able to do it with our own tools, which will be more efficient I think. |
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
|
Thanks |
What kind of change does this PR introduce?
The issue is that currently we test against opaque wasm binary and doesn't help when debugging.
I only converted two tests for the demo. I'll continue if you agree.
Did you add tests for your changes?
Does this PR introduce a breaking change?
What needs to be documented once your changes are merged?