Proposal 1 #2

Daniel-Genkin · 2021-06-07T19:33:49Z

Work in progress

This PR presents a a proposal for a new more user optimized way of interacting with wasm.

It is not targeting dotnet/runtime as it is currently just in the planning/hypothetical stages. The new sample does not run yet.

What's changed:

I created a new browser sample that behaves equally to the already existing one but that is more explicit and more understandable. this is a proposal for how the other samples should be rewritten.

Proposed changes (mostly still in progress)

All JS should be in the .js files
Module will get renamed to DotNet via the emcc EXPORT_NAME flag for clarity (Or should DotNet be something entirely separate?)
User will not need to handle the initialization, loading and setup but will still have the option if needed
~~A js_support.js file exists that can be used to offload this initialization and other processes from the user~~ (unnecessary anymore as the functionality can be contained in library_mono instead)
User can modify the mono configuration at runtime before it is loaded into the runtime
ES6 syntax is used and public API has TypeScript type annotations
Confusing names and inconsistent syntax are minimized
Methods, functions and variables will be camelCase rather than snake_case
The MONO, BINDING and DOTNET namespaces will merged into a single well organized and consistent namespace that will act as the exported WASM API.

To see a full list of changes you can diff with the existing browser sample as they have the same functionality

Note that since this PR was created, I have opened another one that adds TypeScript to the runtime JS files. It can be used as a reference for the types as here they are very general. dotnet#55871

pavelsavara · 2021-06-09T07:07:17Z

src/mono/sample/wasm/browser2/main.js

+
+DotNet.onConfigLoaded = onConfigLoaded; // optional
+DotNet.onRuntimeInitialized = onRuntimeInitialized; // optional
+DotNet.onMonoRuntimeInitialized = onMonoRuntimeInitialized; // required as it acts like the entry point into the program


Let's make this optional too ? If undefined we could call App.init() on behalf of the user.

App.init is just what the old browser sample used (see further down the file). App is entirely user defined and thus not required by us for then to have. Do you think we should make it required? If so, we can make this callback optional.

I think we should keep it as is because I think it is a bit simpler and easier to understand since they have 3 callbacks instead of 3 callbacks + some mandatory object (that can't be renamed) with a mandatory function in it (that also can't be renamed) to choose from.

src/mono/sample/wasm/browser2/Program.cs

pavelsavara · 2021-06-09T07:09:34Z

src/mono/sample/wasm/browser2/main.js

+
+const App = {
+ init: function () {
+ const ret = BINDING.call_static_method("[Wasm.Browser.Sample] Sample.Test:TestMeaning", []);


BINDING should become part of the DotNet import or separate import in DotNet.Binding.

To not pollute global namespace

pavelsavara · 2021-06-09T07:12:35Z

src/mono/sample/wasm/browser2/declarations/interop.d.ts

+
+// TODO find better types than the anys, however it will do for now
+
+export interface BINDING { // Should BINDING be renamed?


Yes, and it should belong to DotNet namespace.
Also all the methods here should be renamed to camelCase as usual in JS.

Also I would like to understand if this is public contract or just internal API for Mono team.

Ok. I also didn't yet add the library_mono.js exported functions nor those from dotnet_support.js. Perhaps I can ask this during the team meeting today.

pavelsavara · 2021-06-09T11:48:43Z

src/mono/sample/wasm/browser2/main.js

+}
+
+// Called when MONO runtime is loaded and ready or if it errors
+function onMonoRuntimeInitialized (error) {


Let's split this. Remove the error parameter and call it only on success.
Let's have another callback onFatalError which could be called any time runtime experienced something terrible.

This would result in 2 required callbacks. Perhaps, we should define some priorities for these changes? Currently I was just trying to make it as elegant as possible but elegant is a subjective term and not overly precise haha. So I was trying to minimize API surface area and make things explicit. Are these the only goals or are there some more as well?

Regardless, I agree with making a onFatalError callback.

onFatalError should be optional too. default behavior console.error if not defined

src/mono/sample/wasm/browser2/main.js

pavelsavara · 2021-06-09T11:52:52Z

src/mono/wasm/runtime/js_support.js

+ configLoaded({error: "Error loading mono-config.json file from current directory"});
+ }
+ } else if (ENVIRONMENT_IS_WEB){
+ const xobj = new XMLHttpRequest();


Larry made note about fetch polyfill.

Yes, I am waiting to see the note, and other comments, before making any changes to make sure I understand what he means. However as this file is loaded before emscripten, I am not sure if the polyfill is ready yet.

pavelsavara · 2021-06-09T11:55:06Z

src/mono/sample/wasm/browser2/main.js

+};
+
+const App = {
+ init: function () {


to make clear this is user space, rename it to customInit or sampleInit

Do we need this App object? I kept it to stay similar to the old browser sample. However, would it be more clear to remove this object and just have a main or init function?

in C# you mean? yes
in JS that is kind of preMain :-)

Daniel-Genkin · 2021-06-09T12:34:40Z

Should the interop.d.ts and runtime.d.ts be moved to the runtime folder?

Daniel-Genkin · 2021-06-09T14:15:42Z

Looks like currently every function in dotnet.js can be called by the samples since they are made publicly available by dotnet.js. Can we do something about this to namespace and make the API more organized? Also, these functions use snake_case but should probably be converted to use camelCase as per the usual JS convention. This could probably be done by refactoring the js files in the runtime folder but does anyone know if there is another way?

+default to es6 and emcc modularize

Proposal 1

b55907d

Daniel-Genkin marked this pull request as draft June 7, 2021 19:34

Daniel-Genkin added 2 commits June 8, 2021 15:09

Some cleanup of propsal 1

49dffa8

added export to DotNet

b9b1a34

pavelsavara reviewed Jun 9, 2021

View reviewed changes

src/mono/sample/wasm/browser2/Program.cs Show resolved Hide resolved

pavelsavara reviewed Jun 9, 2021

View reviewed changes

src/mono/sample/wasm/browser2/main.js Outdated Show resolved Hide resolved

pavelsavara reviewed Jun 9, 2021

View reviewed changes

work in progress - addressed some comments

61ba2b3

Daniel-Genkin added 3 commits June 9, 2021 10:38

added the rest of the rough types from other js files

36ee3f8

moved wasm declarations out of sample

2d58891

fixed typo

3940a4e

Daniel-Genkin mentioned this pull request Jun 9, 2021

[WASM] Converted mono-config.js to mono-config.json dotnet/runtime#53606

Merged

Daniel-Genkin added 2 commits June 9, 2021 16:52

+tsconfig

7023612

+default to es6 and emcc modularize

update to sync with config PR + misc improvements

8b5acc5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal 1 #2

Proposal 1 #2

Daniel-Genkin commented Jun 7, 2021 •

edited

Loading

pavelsavara Jun 9, 2021

Daniel-Genkin Jun 9, 2021 •

edited

Loading

pavelsavara Jun 9, 2021

pavelsavara Jun 9, 2021

pavelsavara Jun 9, 2021

Daniel-Genkin Jun 9, 2021

pavelsavara Jun 9, 2021

Daniel-Genkin Jun 9, 2021

pavelsavara Jun 9, 2021

pavelsavara Jun 9, 2021

Daniel-Genkin Jun 9, 2021

pavelsavara Jun 9, 2021

Daniel-Genkin Jun 9, 2021 •

edited

Loading

pavelsavara Jun 9, 2021

Daniel-Genkin commented Jun 9, 2021

Daniel-Genkin commented Jun 9, 2021


		// TODO find better types than the anys, however it will do for now

		export interface BINDING { // Should BINDING be renamed?

Proposal 1 #2

Are you sure you want to change the base?

Proposal 1 #2

Conversation

Daniel-Genkin commented Jun 7, 2021 • edited Loading

Work in progress

What's changed:

Proposed changes (mostly still in progress)

Choose a reason for hiding this comment

Daniel-Genkin Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daniel-Genkin Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daniel-Genkin commented Jun 9, 2021

Daniel-Genkin commented Jun 9, 2021

Daniel-Genkin commented Jun 7, 2021 •

edited

Loading

Daniel-Genkin Jun 9, 2021 •

edited

Loading

Daniel-Genkin Jun 9, 2021 •

edited

Loading