feat(wasm): Support more targets #848
Conversation
|
One of my biggest concerns with our current WASM package is it is very large in size (it packages all the WASM code twice, and now it will do it for a third time). How big is the package after this change? I wonder if it is better to release these three as independent npm packages |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #848 +/- ##
=======================================
Coverage 97.03% 97.03%
=======================================
Files 16 16
Lines 5995 5995
=======================================
Hits 5817 5817
Misses 178 178 ☔ View full report in Codecov by Sentry. |
I believe the WASM generated by the three methods should be identical (although they are not), with the only difference being the JavaScript glue code. However, this requires improvements in wasm-bindgen. |
|
Current unpacked size is 6.11MB, and indeed it gets increased to 9.2MB, representing 1/3 of the package. 9.2MB is a really big package size imo (6.11MB is already large) Yeah, it is a shame that it is not exactly 1 WASM output shared for each different platform right now. In the output you show, all the WASMs have the same file size, so I'd be curious if there is indeed any difference. Maybe we could optimise it a bit here. |
|
I made another attempt, check here: They share the same WebAssembly file. Some clever hacks were used for the Bundler target. I'm concerned whether these hacks will become a maintenance burden in the future, so I would like to know your opinion. |
|
|
|
All done. |
magic-akari
changed the title
|
This looks pretty awesome, thanks for taking the time! You are right though, I don't completely follow what is happening yet 😅, so might be a pain to maintain in the future. I'm open to merging it in though. We don't have any (smoke)tests to see if the wasm code actually works, so I would want to verify it still works correctly on all the platforms. However, when I was first thinking about this, I was wondering whether it would be as simple as: npx wasm-pack@0.10.3 build --target nodejs --out-dir wasm/stylua.node -- --features lua52,lua53,lua54,luau
npx wasm-pack@0.10.3 build --target bundler --out-dir wasm/stylua.bundler -- --features lua52,lua53,lua54,luau
npx wasm-pack@0.10.3 build --target web--out-dir wasm/stylua.web -- --features lua52,lua53,lua54,luau
rm wasm/stylua.node/stylua_lib_bg.wasm
rm wasm/stylua.bundler/stylua_lib_bg.wasm
# and then some sed-like commands to replace references of stylua_lib_bg.wasm in the above 2 folders to the stylua.web versionDid you happen to see if that was possible? IMO it seems a bit simpler, but unsure whether it is feasible. Also unsure whether it would still support the other new targets you mentioned - so if your work is more flexible I'm happy to keep it as-is |
Unfortunately, this approach does not work. if we open three generated WebAssembly files to examine their contents and observe that their import paths are all different. ( From top to bottom, they are node, web, and bundler. ) 
|
|
The next step is to choose one as the reference and implement the other two. 
When examining three JavaScript glue files, it can be observed that both bundler and node initialization are synchronous. However, web initialization has two options: synchronous and asynchronous, encapsulated within functions. Therefore, choosing web as the foundation makes it easier to implement.
I noticed that the Lines 6 to 8 in 59a6ae1 In the above WebAssembly code, we can observe that the import namespace for the bundler is a JavaScript (js) file. However, for our web build (wbg), we use the Lines 68 to 70 in 59a6ae1 |
|
Reasons for needing the web target: With the web target available, we can use the following approach: import init, { formatCode, Config, IndentType, OutputVerification } from "@johnnymorganz/stylua/web";
export async function fmt_init() {
const wasm_uri = vscode.Uri.joinPath(context.extensionUri, "stylua_lib_bg.wasm");
const bits = await vscode.workspace.fs.readFile(wasm_uri);
await init(bits);
}
export function formattingSubscription() {
return vscode.languages.registerDocumentFormattingEditProvider("lua", {
async provideDocumentFormattingEdits(document, options, token) {
const text = model.getValue();
const indent_type = options.insertSpaces ? IndentType.Spaces : IndentType.Tabs;
const indent_width = options.tabSize;
const config = Config.new().with_indent_type(indent_type).with_indent_width(indent_width);
const formatted = formatCode(text, config, undefined, OutputVerification.None);
const range = document.validateRange(new vscode.Range(document.positionAt(0), document.positionAt(text.length)));
return [vscode.TextEdit.replace(range, formatted)];
}
});
} |
|
I noticed that there is a VSCode extension in the stylua project. For my personal use, I primarily use it on the Monaco Editor. |
If you encounter any issues in the future, feel free to reach out to me. |
|
Thank you for the detailed description! This makes a lot more sense now, much appreciated.
Yep, this is something I have considered before. The main reason why I haven't done this yet is because we allow users to choose different stylua versions and upgrade stylua versions without also upgrading the extension version. I am not sure how to cleanly support this when moving to wasm. (Maybe we could also attach the wasm build on the releases page? But I don't know if vscode.dev supports downloading) |
webtarget for wasm build #803