[WASM] Normalize runtime-test.js #54281
Conversation
|
Tagging subscribers to this area: @directhex Issue DetailsWork in ProgressThis PR aims to reorganize runtime-test.js and add the framework for the follow up PR which will add NodeJS support to WASM! Why is this neededThe What's changed
|
|
/azp run runtime |
|
Commenter does not have sufficient privileges for PR 54281 in repo dotnet/runtime |
|
/azp run runtime |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -49,28 +47,16 @@ if (is_browser) { | |||
| let consoleWebSocket = new WebSocket(consoleUrl); | |||
| consoleWebSocket.onopen = function(event) { | |||
| proxyJson(function (msg) { consoleWebSocket.send (msg); }); | |||
| globalThis.testConsole.log("browser: Console websocket connected."); | |||
| console.log("browser: Console websocket connected."); | |||
There was a problem hiding this comment.
Not opposed to this, but do you know whether there were specific use cases for testConsole that won't be addressed as a result of this change?
There was a problem hiding this comment.
I didn't see any uses for it when I did a search within the repo but I am not entirely sure. Could it be referenced indirectly somehow from C#? Although looks like the tests pass, so it probably isn't used anywhere.
Also this was a change that Larry did in his PR upon which this one is loosely based. @lewing do you know if this is used anywhere besides this file?
Daniel-Genkin
requested review from
BrzVlad,
CoffeeFlux,
EgorBo and
eiriktsarpalis
as code owners
thaystg
removed request for
EgorBo,
eiriktsarpalis,
CoffeeFlux,
BrzVlad,
steveharter,
layomia,
jozkee,
SamMonoRT,
imhameed and
marek-safar
| resolve (response); | ||
| }) | ||
| } | ||
| return IOHandler.fetch (asset, { credentials: 'same-origin' }); |
There was a problem hiding this comment.
I'm pretty sure that when this is used (library-mono.js line 1950) the surrounding try catch block handles this.
try {
if (asset.name === attemptUrl) {
if (ctx.tracing)
console.log ("Attempting to fetch '" + attemptUrl + "'");
} else {
if (ctx.tracing)
console.log ("Attempting to fetch '" + attemptUrl + "' for", asset.name);
}
var fetch_promise = fetch_file_cb (attemptUrl);
fetch_promise.then (handleFetchResponse);
} catch (exc) {
console.error ("MONO_WASM: Error fetching " + attemptUrl, exc);
attemptNextSource ();
}
|
Looking at the changes, don't believe there will be any perf implications. Am I correct ? |
This is mostly just for readability and to add the |
|
Closing PR to save time as the NodeJS PR (#54640 ) includes all these changes |
ghost
locked as resolved and limited conversation to collaborators
Work in Progress
This PR aims to reorganize runtime-test.js and add the framework for the follow up PR which will add NodeJS support to WASM!
Why is this needed
The
runtime-test.jsfile is currently an amalgamation of functions and glue code that makes it hard to understand and hard to work with. The primary goal of this PR is to essentially refactor it such that it maintains the same functionality but in a more readable way. The most heavily affected aspect of the file is the way IO is handled. The secondary goal is to add theis_nodevariable along withrequireand other node specific methods for doing various operations which would be useful later on when adding NodeJS support to WASM.What's changed
runtime-test.jsIOHandlerobject which abstracts all IO related operations and offers simple asyncload,readandfetchfunctions.varhave been updated to be eitherletorconstruntime-test.js