Skip to content
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] rationalize internal exports #60075

Merged
merged 6 commits into from
Oct 14, 2021
Merged

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Oct 6, 2021

  • reduce BINDING and MONO exports to minimal scope necessary - as used by Blazor
  • introduce globalThis.INTERNAL and move all exported methods which we only use internally or for testing
  • fix all internal usages in tests
  • produce dotnet.d.ts and include it in the workload
  • moved Module.config to MONO.config
  • added mono_load_runtime_and_bcl_args into MONO export
  • add mono_wasm_new_root_buffer, mono_wasm_new_root, mono_wasm_release_roots to the MONO export
  • removed obsolete debugger test InvalidScopeId
  • introduced INTERNAL.mono_wasm_set_main_args

@pavelsavara pavelsavara added the arch-wasm WebAssembly architecture label Oct 6, 2021
@pavelsavara pavelsavara added this to the 7.0.0 milestone Oct 6, 2021
@pavelsavara pavelsavara requested review from kg and thaystg October 6, 2021 18:06
@dotnet-issue-labeler
Copy link

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.

@ghost
Copy link

ghost commented Oct 6, 2021

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: pavelsavara
Assignees: -
Labels:

arch-wasm

Milestone: 7.0.0

@kg
Copy link
Contributor

kg commented Oct 6, 2021

Cleaning up the 'Module' goo is good, but i feel like INTERNAL is probably the wrong name especially if we end up touching it from tests, etc.

@pavelsavara
Copy link
Member Author

Cleaning up the 'Module' goo is good, but i feel like INTERNAL is probably the wrong name especially if we end up touching it from tests, etc.

I don't insist on INTERNAL. Do you have other candidates on mind @kg ?

…e only use internally or for testing

- reduce BINDING and MONO exports to minimal scope necessary - as used by Blazor
- fix all internal usages in tests
- produce dotnet.d.ts and include it in the workload
- moved Module.config to MONO.config
- added mono_load_runtime_and_bcl_args into MONO export
- removed obsolete debugger test InvalidScopeId
- introduced INTERNAL.mono_wasm_set_main_args
@pavelsavara pavelsavara marked this pull request as ready for review October 12, 2021 16:58
@pavelsavara
Copy link
Member Author

@jeromelaban could you please let me know if this change would break any of Uno's use cases.
Are you using some of the methods which originally were on MONO or BINDING and which I unpublished ?
Please see

export interface MONO {

@pavelsavara
Copy link
Member Author

cc @SteveSandersonMS @mkArtakMSFT, this PR should have no impact to Blazor, but perhaps you could share your comments.

@jeromelaban
Copy link
Contributor

Thanks @pavelsavara. Uno can cope with the change, but many extensions or projects (not just uno-based) are using this particular one:

https://github.com/search?q=mono_bind_static_method&type=Code

@kg
Copy link
Contributor

kg commented Oct 12, 2021

Thanks @pavelsavara. Uno can cope with the change, but many extensions or projects (not just uno-based) are using this particular one:

https://github.com/search?q=mono_bind_static_method&type=Code

I personally think bind_static_method should be public

@pavelsavara
Copy link
Member Author

Thanks @pavelsavara. Uno can cope with the change, but many extensions or projects (not just uno-based) are using this particular one:

https://github.com/search?q=mono_bind_static_method&type=Code

Thanks for the hint. Using it via Module is pretty bad.
I would like to get rid of Module from the global namespace in the future (emscripten output as es6 module).
Do people also use emscripten low level features, like memory manipulation or FS ?

@kg
Copy link
Contributor

kg commented Oct 12, 2021

Blazor uses Module._malloc, but we could expose our own blessed version of it via MONO. instead. I think people would have to be using FS right now to do various things, maybe we should expose an API for that as well (though they can load things ahead of time via mono-config)

@jeromelaban
Copy link
Contributor

Thanks @pavelsavara. Uno can cope with the change, but many extensions or projects (not just uno-based) are using this particular one:
https://github.com/search?q=mono_bind_static_method&type=Code

Thanks for the hint. Using it via Module is pretty bad. I would like to get rid of Module from the global namespace in the future (emscripten output as es6 module). Do people also use emscripten low level features, like memory manipulation or FS ?

It happens yes (both memory manipulation and FS). SkiaSharp does memory manipulation for instance, Uno as well.

Modularizing would be very nice indeed.

Is this for net6 or net7 ?

@pavelsavara
Copy link
Member Author

pavelsavara commented Oct 12, 2021

Is this for net6 or net7 ?

net7

@pavelsavara
Copy link
Member Author

I personally think bind_static_method should be public

Module.mono_bind_static_method is the same as BINDING.bind_static_method so it's already published.

I will keep Module.mono_bind_static_method in place for compat for now. No promise about future PRs ;)

@kg
Copy link
Contributor

kg commented Oct 12, 2021

I personally think bind_static_method should be public

Module.mono_bind_static_method is the same as BINDING.bind_static_method so it's already published.

I will keep Module.mono_bind_static_method in place for compat for now. No promise about future PRs ;)

We could make all the Module accessors produce a warning or error in console on first (all?) uses so people fix them

@pavelsavara
Copy link
Member Author

I added console.warn to each call of the Module.mono_bind_static_method and I removed all the other methods from Module which I forgot to remove before.

Obviously all 29 of the extern C methods which emcc linker needs to see are there as _mono_wasm* but that could not be helped at this time. Perhaps after we modularize.

@kg kg merged commit a546fa4 into dotnet:main Oct 14, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Nov 15, 2021
@pavelsavara pavelsavara deleted the wasm_js_exports branch January 4, 2022 13:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants