Difficulty using Set - members of Jint.Native.JsSet are not public

[kenlyon]

Version used

4.0.3

Describe the bug

I'm trying to create a new Set() in JavaScript. This creates a Jint.Native.JsSet. I expect to be able to use methods like add() but they all seem to be internal.

To Reproduce

I had one expression which returns a new Set() and then a subsequent one that attempted to call .add() on the object that had been returned from the first one. I had been using .ToObject() on the result of the first expression to get a .NET type, so it was technically a System.Dynamic.ExpandoObject.

Expected behavior

I expect the methods Set to be available in a Jint expression.

Additional context

I do see there is a Jint.Native.Set type that seems to expose more of the members that I might have expected. However, I don't want to have to hard-code any special handling of Set. It is my understanding that Jint is meant to provide the equivalent APIs of the JavaScript types without any additional handling.

[lahma]

Would you like to propose a PR accompanied by tests in public API test project to ensure API promise?

[kenlyon]

@lahma Not really. I’m asking how it’s meant to work. Is it expected that the JsSet class is used here?

[lahma]

Well basically I think there should probably be an API like we have ArrayBuffer:

jint/Jint.Tests.PublicInterface/ArrayBufferTests.cs

Lines 36 to 37 in 7857b9c

	var jsArrayBuffer = engine.Intrinsics.ArrayBuffer.Construct(1);
	var jsTypedArray = engine.Intrinsics.Uint8Array.Construct(jsArrayBuffer);

So both Set and Map would fall into category of calling engine.Intrinsics.Set.Construct() and engine.Intrinsics.Map.Construct() with idiomatic parameters (either none or some collection of existing items). Then both types and minimal set of APIs could be exposed by changing internal to public.

[kenlyon]

The way I'm using Jint, the JavaScript being evaluated is dynamically provided at runtime, so I wouldn't be using engine.Intrinsics directly.

I've checked out the repo and experimented with it. Even if I change the members of JsSet to public, they are still not visible when I do this:

var engine = new Engine();
engine.Evaluate("var s = new Set()");
var s = engine.GetValue("s");

I'm also having trouble creating instances of classes within the Intl namespace, which is listed as supported. I might need to log that separately. I'm hoping to be able to evaluate something like new Intl.Collator() but it says "Intl is not defined".

[lahma]

var engine = new Engine();
engine.Evaluate("var s = new Set()");
var s = engine.GetValue("s");

Not sure what you are after here..

I'm also having trouble creating instances of classes within the Intl namespace, which is listed as supported. I might need to log that separately. I'm hoping to be able to evaluate something like new Intl.Collator() but it says "Intl is not defined".

Listed as supported, where? They would be a huge undertaking, but PRs welcome of course.

[kenlyon]

In my first snippet, the s variable is an instance of Jint.Native.JsSet (at least if I inspect s.GetType().FullName. Despitie this, s.Add() is not a visible method on the object even after I changed Add to be public.

Sorry, I was mistaken about the Intl namespace. I double-checked and it's not listed in the Supported features section after all. I think I just discovered Jint.Native.Intl.IntlInstance and assumed it would work because all of the types had been defined. If that's not the case I can skip it, though.

[lahma]

I think that if you follow the example I linked how native constructs are being constructed in a PR backed with public tests, that would a great addition.

[kenlyon]

I think we're talking at cross purposes here. I understand you see the value in copying the existing patterns for consistency and to add some more code coverage.

My concern is that the changes you're suggesting don't seem to help me. They don't make the members visible in my use case. I expected that changing the accessor on Jint.Native.JsSet.Add() to public would make it visible in the object I get back but it doesn't. This is the specific thing I'm interested in solving.

Also, more broadly I think that the way I'm using Jint isn't the way you're expecting it to be, based on the tests I see. For example, I see that the NegativeZeroKeyConvertsToPositiveZero test in Jint.Tests/Runtime/SetTests.cs calls the add() method successfully. I assume this works because the method is called in the same script where the set is declared. Most of my use cases are not like that. It's far more common for me to have one script that constructs and returns something and then subsequent scripts that interact with it. Is this a use case that you would expect to work?

[lahma]

I think it would greatly clarify things if you could submit a PR with test cases you would like to see succeed. You can alter any APIs you need for it to compile of course.

[kenlyon]

I'm starting to get frustrated with this repeated discussion about PRs. I will not add a pull request as I don't know what changes are necessary. I am asking for your help!

Here is a unit test that roughly demonstrates how I use Jint and expect it to work:

[Fact]
public void TestingAddFromSeparateScript()
{
    // Create the set
    var mySet = new Engine()
        .Evaluate("return new Set()")
        .ToObject();

    // Add a value to the set.
    new Engine()
        .SetValue("mySet", mySet)
        .Evaluate("mySet.add(123)");

    // Check if the value is in the set.
    var result = new Engine()
        .SetValue("mySet", mySet)
        .Evaluate("mySet.has(123)")
        .ToObject();

    Assert.True((bool) result);
}

The second .Evaluate() call fails with this error:

Jint.Runtime.JavaScriptException : No public methods with the specified arguments were found.
---- Jint.Runtime.JavaScriptException+JavaScriptErrorWrapperException : No public methods with the specified arguments were found.

Other code changes I made trying to get this to work:

• Changed Jint.Native.Set.SetConstructor from internal to public.
• Changed all internal members of Jint.Native.JsSet to public.
• Changed Jint.Runtime.Intrinsics.Set from internal to public.

My priority is to learn:

• Does Jint support what I am trying to do, or have I misunderstood what extent these features of JavaScript are supported?
• If not, should it?

I am not volunteering to dig through your codebase and try to fix it for you. I'm asking what's actually expected to work.

[lahma]

Please do understand that when someone asks something to be green and ideal for consumption, it's really hard to understand the usage scenario. Thank you for bringing some code into the table to investigate. I'll try to find some time to dig into this.

[lahma]

Your test case passes if you remove the .ToObject() call from the set creation. ToObject() is meant to be used when you want a CLR wrapped instance out of engine, if you don't call it you will get the JsValue (JsSet) which will have the original API surface.

[kenlyon]

@lahma Thanks for your help and sorry for the confusion along the way. I think we got there. It seems then all I need to do is avoid using .ToObject(), at least in cases like this. I might need to be careful with my use cases to avoid regressions but that's something for me to figure out.

Now that I see a way to get the members of Set to work, I'm not blocked any more.

Thanks again!

[lahma]

Great that this is now sorted. I think that ideally if you have the need to add items on c# side, you would cast to JsSet and call mySet.Add as it would be far more performant - that was the original story I was thinking of.

[kenlyon]

That makes sense. The challenge I face is that our product is used by customers who link together separate steps in a flowchart type structure that are evaluated separately. They could put any JavaScript in these and also refer back to the output of previous steps. We don't know in advance what their scripts are going to be so we need a generic enough solution that works in as many cases as possible.

For most cases, the outputs of the expression are most likely only going to be re-used in subsequent Jint expressions, although I will need to check to be sure. There might be cases where things are also passed through to C# code.

I could still try the casting like you suggest, although I might need to somehow inspect the object at runtime to determine the type it should be cast to. I'm not sure if there would be a dynamic way to achieve that.

[kenlyon]

I discovered it’s not possible to cast to JsSet as the class is internal. This does seem to be a slight inconsistency. I have worked around it, though.

I’ve made my code charges and written some unit tests. I use .GetType().FullName to check if the result of my expression is a Jint.Native.JsSet. In that one case I no longer call .ToObject() in the result.

This allows a set to be reused in a subsequent expression without affecting my application’s existing behaviour for other types. That is the lowest risk of regressions.

Thanks again for your help. I’m glad we found a solution.

[kenlyon]

@lahma I ended up thinking it might be good to be able to convert a JsSet to a HashSet to avoid using Jint types elsewhere. To that end, I've created a PR to make JsSet public:

#1987