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

Add support for wrapped lists to push and pop items #1822

Merged
merged 1 commit into from
Apr 1, 2024

Conversation

ejsmith
Copy link
Contributor

@ejsmith ejsmith commented Mar 30, 2024

Test that is showing that trying to push to an interop list does not work. Should this work? Is there some rule that I'm not aware of that is trying to keep interop to read only operations?

@lahma
Copy link
Collaborator

lahma commented Mar 31, 2024

Jint doesn't automatically attach array prototype when wrapping objects (same as with STJ). Here's a good example which it would a bad decision by default. JS expects that you can index outside of collection size to create a new entry. Probably a custom ObjectWrapper implementation catching the outside-of-bounds indexer usage and calling Add instead for target might work.

@ejsmith
Copy link
Contributor Author

ejsmith commented Mar 31, 2024

Hmm... I guess that is kind of confusing that it implements things like indexOf but not push. I will experiment with a IObjectConverter to see if I can make it support push.

@lahma
Copy link
Collaborator

lahma commented Mar 31, 2024

IndexOf is a List method, it doesn't need JS array prototype

@ejsmith
Copy link
Contributor Author

ejsmith commented Mar 31, 2024

Hmm... yeah, I guess a lot of the JS Array methods line up with the list methods. I tried what you said and it didn't work either.

        [Fact]
        public void ArrayPrototypePushWithInteropList()
        {
            var engine = new Jint.Engine(options =>
            {
                // make List behave like JS array
                options.Interop.WrapObjectHandler = static (e, target, type) =>
                {
                    if (target is IList)
                    {
                        var wrapped = ObjectWrapper.Create(e, target);
                        wrapped.Prototype = e.Intrinsics.Array.PrototypeObject;
                        return wrapped;
                    }

                    return ObjectWrapper.Create(e, target);
                };
            });

            engine.SetValue("list", new List<string> { "A", "B", "C" });

            engine.Evaluate("list.push('D')");
            Assert.Equal(3, engine.Evaluate("list.lastIndexOf('D')"));
        }

@lahma
Copy link
Collaborator

lahma commented Mar 31, 2024

You probably need to implement something custom deriving from ObjectWrapper which would intercept problematic calls.

@ejsmith
Copy link
Contributor Author

ejsmith commented Mar 31, 2024

ObjectWrapper is sealed.

@ejsmith
Copy link
Contributor Author

ejsmith commented Mar 31, 2024

This seems like something that should work out of the box. Do you agree with that or is against the direction you want to go with the project?

I am going to investigate more and try and figure out the correct spot to implement this. The code is quite complex so trying to figure out the correct way to implement it will probably take some time. I'd be willing to send some sponsor money your way to either give me a tour of the code and point me in the right direction or if you just want to implement yourself.

@ejsmith ejsmith changed the title Add array push failing test Add support for wrapped lists to push and pop items Mar 31, 2024
@ejsmith
Copy link
Contributor Author

ejsmith commented Mar 31, 2024

Kept digging and found a way that I think might be acceptable to get push and pop on wrapped IList objects.

@ejsmith ejsmith force-pushed the main branch 2 times, most recently from 3979170 to c877dfc Compare March 31, 2024 20:59
Jint.Tests/Runtime/InteropTests.cs Outdated Show resolved Hide resolved
Jint.Tests/Runtime/InteropTests.cs Outdated Show resolved Hide resolved
Jint/Native/Array/ArrayOperations.cs Show resolved Hide resolved
Jint/Native/Array/ArrayOperations.cs Outdated Show resolved Hide resolved
Jint/Native/Array/ArrayOperations.cs Outdated Show resolved Hide resolved
Jint/Native/Array/ArrayOperations.cs Outdated Show resolved Hide resolved
Jint/Native/Object/ObjectInstance.cs Outdated Show resolved Hide resolved
@ejsmith
Copy link
Contributor Author

ejsmith commented Apr 1, 2024

Updated. Thanks for the feedback!

Copy link
Collaborator

@lahma lahma left a comment

Choose a reason for hiding this comment

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

Thank you!

@lahma lahma merged commit 3209ab3 into sebastienros:main Apr 1, 2024
3 checks passed
@ejsmith
Copy link
Contributor Author

ejsmith commented Apr 1, 2024

Thank you!

No, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants