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] Revert back to eval in wasm InvokeJS with modularization support . #61212

Merged
merged 3 commits into from
Nov 5, 2021

Conversation

maraf
Copy link
Member

@maraf maraf commented Nov 4, 2021

Wrap code to evaluate in a function with MONO, BINDING, INTERNAL
and module as local variables. Than use eval.

Added tests for invoking js expressions.

Fixes #61192.

@maraf maraf added this to the 7.0.0 milestone Nov 4, 2021
@maraf maraf requested review from lewing and pavelsavara November 4, 2021 15:04
@ghost
Copy link

ghost commented Nov 4, 2021

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

Issue Details

Wrap code to evaluate in a function with MONO, BINDING, INTERNAL
and module as local variables. Than use eval.

Fixes #61192.

Author: maraf
Assignees: -
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript

Milestone: 7.0.0

@pavelsavara pavelsavara requested a review from kg November 4, 2021 18:50
@pavelsavara pavelsavara marked this pull request as ready for review November 4, 2021 18:51
@pavelsavara pavelsavara marked this pull request as draft November 4, 2021 18:53
@pavelsavara
Copy link
Member

Tests failed with
Return type of InvokeJS is string. Can't marshal response of type object.
We also need to do res = res.toString (); as in original code.

@kg
Copy link
Member

kg commented Nov 4, 2021

Tests failed with Return type of InvokeJS is string. Can't marshal response of type object. We also need to do res = res.toString (); as in original code.

@maraf if you want to run the bindings tests specifically when iterating on APIs like this, this will run a sizable (but not complete!) chunk of them:

~/Projects/dotnet-runtime-wasm$ make -C src/mono/wasm runtime corlib CONFIG=Debug && clear &&  ./dotnet.sh build /t:Test /p:TargetOS=Browser /p:TargetArchitecture=wasm /p:Configuration=Debug ~/Projects/dotnet-runtime-wasm/src/libraries/System.Private.Runtime.InteropServices.JavaScript/tests/

(replace the folder with the location of your checkout)

@pavelsavara
Copy link
Member

Also, please add the test case which @kg mentioned in the issue.

@pavelsavara
Copy link
Member

pavelsavara commented Nov 4, 2021

Windows incantation for the test same is

build.cmd -bl -os Browser -subset mono.corelib+mono.wasmruntime -c Debug
dotnet.cmd build /bl:test.binlog /p:TargetOS=Browser /p:TargetArchitecture=wasm /p:Configuration=Debug /t:Test src/libraries/System.Private.Runtime.InteropServices.JavaScript/tests

Wrap code to evaluate in a function with MONO, BINDING, INTERNAL
and module as local variables.

Added tests for running js expressions.

Co-authored-by: Pavel Savara <[email protected]>
@maraf maraf force-pushed the wasm_fix61192_invokejs branch from 3de1205 to c952f06 Compare November 4, 2021 20:06
@dnfadmin
Copy link

dnfadmin commented Nov 4, 2021

CLA assistant check
All CLA requirements met.

@kg
Copy link
Member

kg commented Nov 4, 2021

Can you add a test that verifies the eval is not happening in global scope? I think it would invoke some JS like:

(test_local_variable_name = 5, globalThis.test_local_variable_name)

InvokeJS on that will return 5 in global scope, and return undefined otherwise

@maraf
Copy link
Member Author

maraf commented Nov 4, 2021

@kg Was it intended to use an undeclared variable? As we are in the strict mode, one can't use undeclared variable. I can add a test with variable declaration, something like

var test_local_variable_name = 5; globalThis.test_local_variable_name

Add test ensuring global scope separation.
@maraf maraf marked this pull request as ready for review November 5, 2021 14:00
@maraf maraf merged commit 39a4a70 into dotnet:main Nov 5, 2021
@maraf maraf deleted the wasm_fix61192_invokejs branch November 5, 2021 14:31
@pavelsavara
Copy link
Member

@kg @maraf guys, I think we should reconsider again. The InvokeJS is internal API and the version without eval is breaking just edge case scenario. On the other hand, we have to look at that compilation warning every day.

@kg
Copy link
Member

kg commented Dec 6, 2021

If the only motivation is the compile warning, we can suppress the warning.

@maraf
Copy link
Member Author

maraf commented Dec 6, 2021

I don't see a good reason for using function instance.

  • Both are basically eval (with all the bad things that come with it).
  • The code is always evaluated just once.
  • For better perf, users should write a function and call it.

@ghost ghost locked as resolved and limited conversation to collaborators Jan 5, 2022
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.

wasm InvokeJS return value is broken
4 participants