Skip to content

[wasm] Preparation for public API in System.Runtime.InteropServices.JavaScript #71332

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 30, 2022
Merged

[wasm] Preparation for public API in System.Runtime.InteropServices.JavaScript #71332

merged 3 commits into from
Jun 30, 2022

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Jun 27, 2022 •
edited
Loading

In preparation for #66304 making the overall PR smaller and easier to review.

  • new ref assembly System.Runtime.InteropServices.JavaScript - empty
  • new src assembly System.Runtime.InteropServices.JavaScript
    • moved all implementation from System.Private.Runtime.InteropServices.JavaScript into it
    • refactoring of Interop.Runtime, System.Runtime.InteropServices.JavaScript.Runtime classes into JavaScriptExports, JavaScriptImports, JSHostImplementation
    • making JSObject slimmer and movig legacy API to legacy extension methods
    • marking legacy classes [Obsolete] and excluding them from ref assembly
  • added IMPORTS, EXPORTS to js API
  • refactored setup_managed_proxy and teardown_managed_proxy in JS
  • added more range assert for working with wasm memory in JS

@pavelsavara pavelsavara added this to the 7.0.0 milestone Jun 27, 2022
@pavelsavara pavelsavara self-assigned this Jun 27, 2022
@ghost
Copy link

ghost commented Jun 27, 2022

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jun 27, 2022

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

Issue Details
  • new ref assembly System.Runtime.InteropServices.JavaScript - empty
  • new src assembly System.Runtime.InteropServices.JavaScript
    • moved all implementation from System.Private.Runtime.InteropServices.JavaScript into it
    • refactoring of Interop.Runtime, System.Runtime.InteropServices.JavaScript.Runtime classes into JavaScriptExports, JavaScriptImports, JSHostImplementation
    • making JSObject slimmer and movig legacy API to legacy extension methods
    • marking legacy classes [Obsolete] and excluding them from ref assembly
  • added IMPORTS, EXPORTS to js API
  • refactored setup_managed_proxy and teardown_managed_proxy in JS
  • added more range assert for working with wasm memory in JS
Author: pavelsavara
Assignees: pavelsavara
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript
Milestone: 7.0.0

maraf
maraf reviewed Jun 27, 2022
View reviewed changes
Copy link
Member

@maraf maraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just minor questions if it should be included here. It can be ignored.

@elringus elringus mentioned this pull request Jun 27, 2022
Closed
@pavelsavara pavelsavara force-pushed the wasm_jsimport_jsexport_prep2 branch from 25de5d7 to 97ac91c Compare June 28, 2022 15:59
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara requested a review from maraf June 29, 2022 06:22
@pavelsavara pavelsavara marked this pull request as ready for review June 29, 2022 06:24
@pavelsavara pavelsavara requested a review from marek-safar as a code owner June 29, 2022 06:24
@pavelsavara pavelsavara force-pushed the wasm_jsimport_jsexport_prep2 branch from 97ac91c to 24709b9 Compare June 29, 2022 10:47
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

Failures are unrelated

radical
radical reviewed Jun 29, 2022
View reviewed changes
Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed whatever bits I could.

@pavelsavara pavelsavara force-pushed the wasm_jsimport_jsexport_prep2 branch 2 times, most recently from c4c165d to d61abb8 Compare June 29, 2022 21:11
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@pavelsavara pavelsavara requested a review from radical June 29, 2022 21:39
@radical
Copy link
Member

radical commented Jun 29, 2022 •
edited
Loading

Do you know how to see the diff between the last state of the PR, when I had reviewed, and now? Force push changes the history, so I can't see new changes.

@pavelsavara
Copy link
Member Author

Do you know how to see the diff between the last state of the PR, when I had reviewed, and now? Force push changes the history, so I can't see new changes.

Sorry, https://github.com/dotnet/runtime/compare/24709b98f4b9cc16f1300b1015efd97bc21e3dcd..d61abb8189194c6d926a21776561231db9434fa3

kg
kg reviewed Jun 29, 2022
View reviewed changes
src/libraries/System.Runtime.InteropServices.JavaScript/src/Resources/Strings.resx
@@ -1,4 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<?xml version="1.0" encoding="utf-8"?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen invisible changes on the first line of a few files in this diff, what are they? Byte order markers?

kg
kg reviewed Jun 29, 2022
View reviewed changes
kg
kg reviewed Jun 29, 2022
View reviewed changes
kg
kg reviewed Jun 29, 2022
View reviewed changes
kg
kg reviewed Jun 30, 2022
View reviewed changes
kg
kg reviewed Jun 30, 2022
View reviewed changes
kg
kg reviewed Jun 30, 2022
View reviewed changes
kg
kg reviewed Jun 30, 2022
View reviewed changes
kg
kg reviewed Jun 30, 2022
View reviewed changes
kg
kg reviewed Jun 30, 2022
View reviewed changes
kg
kg reviewed Jun 30, 2022
View reviewed changes
kg
kg approved these changes Jun 30, 2022
View reviewed changes
Copy link
Member

@kg kg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine, there are a couple changes I'd like to see (mentioned above) but nothing remotely worthy of blocking this

@pavelsavara
rebase, squash
606cde2
@pavelsavara
feedback
36a32a3 
Co-authored-by: Ankit Jain <radical@gmail.com>
Co-authored-by: Marek Fišera <mara@neptuo.com>
@pavelsavara
Co-authored-by: Katelyn Gadd <kg@luminance.org>
3ac6060
@pavelsavara pavelsavara force-pushed the wasm_jsimport_jsexport_prep2 branch from c6923f5 to 3ac6060 Compare June 30, 2022 10:55
@pavelsavara pavelsavara merged commit 4823df3 into dotnet:main Jun 30, 2022
@pavelsavara pavelsavara deleted the wasm_jsimport_jsexport_prep2 branch July 14, 2022 20:46
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@radical radical radical approved these changes

@kg kg kg approved these changes

@maraf maraf maraf approved these changes

@lambdageek lambdageek Awaiting requested review from lambdageek

@radekdoulik radekdoulik Awaiting requested review from radekdoulik

@marek-safar marek-safar Awaiting requested review from marek-safar

Assignees

@pavelsavara pavelsavara

Labels
arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@pavelsavara @radical @kg @maraf