-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[Wasm] JS modularization #61313
[Wasm] JS modularization #61313
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
Tagging subscribers to 'arch-wasm': @lewing Issue Detailswork in progress 7
|
4a9b019
to
9649380
Compare
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.
Looks great, thanks for doing all this work! You don't need to make all the changes I've requested here
They are both receiving the same API object instances, i.e. same The callback could also be written with You can think about it as if The functionality in the The |
Currently this corrupts some of the xharness |
9649380
to
a43160b
Compare
I added |
test failure |
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.
Looks good to me, assuming no one else finds any problems!
@radical please review the msbuild parts. |
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 only have a slight feeling that from user point of view, this syntax may seem a bit odd.
export const { MONO, BINDING } = await createDotnetRuntime(({ MONO, BINDING, Module }) => ...);
I mean "what is the difference between left MONO and right MONO?"
Anyway, I can't find a better solution.
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.
Reviewed the parts that I could
The |
The AOT
Also, the app then incorrectly exits with exit code 0, thus it records as tests having passed. This should be fixed so if such an error occurs, then it returns a non-zero exit code. |
Also, make sure to run all the wasm tests. A lot of them are disabled now for PRs, and run fully only for rolling builds, generally conditioned with And cc @thaystg to confirm that debugging still works, and the tests pass too. |
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 reviewed the parts that I could (so, skipped js, and ts!). looking good.
Just some suggestions.
@@ -7,7 +7,7 @@ var Module = { | |||
onConfigLoaded: function () { | |||
MONO.config.environment_variables["DOTNET_MODIFIABLE_ASSEMBLIES"] = "debug"; | |||
}, | |||
onDotNetReady: function () { | |||
onDotnetReady: function () { |
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.
why the change? DotNet
is the style used usually, including package names.
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.
my quick survey told me Dotnet is more common, but I don't have strong opinion.
@lewing this would stick with us going forward, what should it be ?
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 will eventually rename it in next PR
<DebugSymbols>true</DebugSymbols> | ||
<DebugType>embedded</DebugType> | ||
<WasmDebugLevel>1</WasmDebugLevel> | ||
<WasmBuildNative>true</WasmBuildNative> |
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.
We should remove it for this PR, then. And reverse the property setting here, and in the es6 sample, when es6 becomes default.
LINK_DEPENDS "${NATIVE_BIN_DIR}/src/emcc-default.rsp;${NATIVE_BIN_DIR}/src/cjs/dotnet.cjs.pre.js;${NATIVE_BIN_DIR}/src/cjs/runtime.cjs.iffe.js;${NATIVE_BIN_DIR}/src/cjs/dotnet.cjs.lib.js;${NATIVE_BIN_DIR}/src/pal_random.lib.js;${NATIVE_BIN_DIR}/src/cjs/dotnet.cjs.post.js;${NATIVE_BIN_DIR}/src/cjs/dotnet.cjs.extpost.js;" | ||
LINK_FLAGS "@${NATIVE_BIN_DIR}/src/emcc-default.rsp ${CONFIGURATION_LINK_FLAGS} -DENABLE_NETCORE=1 --extern-pre-js ${NATIVE_BIN_DIR}/src/cjs/runtime.cjs.iffe.js --pre-js ${NATIVE_BIN_DIR}/src/cjs/dotnet.cjs.pre.js --js-library ${NATIVE_BIN_DIR}/src/cjs/dotnet.cjs.lib.js --js-library ${NATIVE_BIN_DIR}/src/pal_random.lib.js --post-js ${NATIVE_BIN_DIR}/src/cjs/dotnet.cjs.post.js --extern-post-js ${NATIVE_BIN_DIR}/src/cjs/dotnet.cjs.extpost.js " |
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 list here also should be extracted into variables. Doesn't have to be in this PR.
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.
@radekdoulik how could I do this ? I'll do it in next PR.
Co-authored-by: Ankit Jain <radical@gmail.com>
Co-authored-by: Ankit Jain <radical@gmail.com>
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.
Reviewed the bits that I could, and those look good to me. great job 👍
/azp run runtime-manual |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
debugger-main.js
still usesonDotNetReady
(capital N)
/azp run runtime-manual |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-manual |
Azure Pipelines successfully started running 1 pipeline(s). |
Some of the wasm.build.tests have:
|
|
/azp run runtime-manual |
Azure Pipelines successfully started running 1 pipeline(s). |
Modularized dotnet.js
MODULARIZE
andEXPORT_ES6
, depending onWasmEnableES6
to produce ES6 or CommonJS modules respectively.WasmEnableES6
enablesWasmBuildNative
becausedotnet.js
need to be re-linkedes6/*.js
andes6/*.js
files which are included indotnet.js
creation. Details are described inmodularize-dotnet.md
createDotnetRuntime
method instead of globalModule
src\mono\sample\wasm\browser-es6\runtime.js
createDotnetRuntime(factory: (exports) => DotnetModuleConfig)
takes function which creates
Module
-like config.createDotnetRuntime
returnsPromise<{MONO, INTERNAL, BINDING, Module}>
exports.Module
extensionDotnetModuleConfig
has following new fieldsdisableDotNet6Compatibility
- whentrue
and if runtime script is loaded as module, it would not pollute global namespace.configSrc
- location ofmono-config.json
as defined byMonoConfig
.ts typeonConfigLoaded
- callback called early during runtime init, whenMonoConfig
was loaded but runtime didn't start yet.onDotnetReady
- callback called when runtime is ready and all artifacts were loaded fromMonoConfig
. Or not called at all whenonRuntimeInitialized
is overriden in Blazor.src/mono/wasm/runtime/modularize-dotnet.md
of this PR.Other changes
dotnet.d.ts
it is now generated into version control too, so that we could observe how it evolves.--testing
argument and simplifiedis_testing
logic in the the samples.browser-es6
sample app, which usesWasmEnableES6=true
to compiledotnet.js
as ES6 module.browser-legacy
sample, which usesdotnet.js
CommonJS module and loads it into global namespace.package.json
into the nupkgOut of scope
NodeJs Usage after the change