Create separate ObjectWrapper.Create factory method (#1810)

sebastienros · Mar 16, 2024 · 8078b4a · 8078b4a
1 parent d306162
commit 8078b4a
Show file tree

Hide file tree

Showing 12 changed files with 32 additions and 25 deletions.
diff --git a/Directory.Packages.props b/Directory.Packages.props
@@ -21,7 +21,7 @@
     <PackageVersion Include="NUnit3TestAdapter" Version="4.5.0" />
     <PackageVersion Include="SharpZipLib" Version="1.4.0" />
     <PackageVersion Include="Spectre.Console.Cli" Version="0.45.0" />
-    <PackageVersion Include="System.Text.Json" Version="6.0.8" />
+    <PackageVersion Include="System.Text.Json" Version="8.0.3" />
     <PackageVersion Include="Test262Harness" Version="1.0.0" />
     <PackageVersion Include="xunit" Version="2.7.0" />
     <PackageVersion Include="xunit.runner.visualstudio" Version="2.5.7" PrivateAssets="all" />

diff --git a/Jint.Benchmark/ListInteropBenchmark.cs b/Jint.Benchmark/ListInteropBenchmark.cs
@@ -49,8 +49,8 @@ public void Setup()
             options
                 .SetWrapObjectHandler((engine, target, type) =>
                 {
-                    var instance = new ObjectWrapper(engine, target);
-                    var isArrayLike = IsArrayLike(instance.Target.GetType());
+                    var instance = ObjectWrapper.Create(engine, target);
+                    var isArrayLike = IsArrayLike(target.GetType());
                     if (isArrayLike)
                     {
                         instance.Prototype = engine.Intrinsics.Array.PrototypeObject;

diff --git a/Jint.Tests.PublicInterface/InteropTests.SystemTextJson.cs b/Jint.Tests.PublicInterface/InteropTests.SystemTextJson.cs
@@ -3,10 +3,10 @@
 using Jint.Native;
 using Jint.Runtime.Interop;
 using System.Text.Json;
+
 namespace Jint.Tests.PublicInterface;
 
-#if NET8_0_OR_GREATER
-public class TestJsonValueConverter : IObjectConverter
+public sealed class SystemTextJsonValueConverter : IObjectConverter
 {
     public bool TryConvert(Engine engine, object value, out JsValue result)
     {
@@ -95,15 +95,15 @@ public void AccessingJsonNodeShouldWork()
             {
                 if (target is JsonArray)
                 {
-                    var wrapped = new ObjectWrapper(e, target);
+                    var wrapped = ObjectWrapper.Create(e, target);
                     wrapped.Prototype = e.Intrinsics.Array.PrototypeObject;
                     return wrapped;
                 }
 
-                return new ObjectWrapper(e, target);
+                return ObjectWrapper.Create(e, target);
             };
 
-            options.AddObjectConverter(new TestJsonValueConverter());
+            options.AddObjectConverter(new SystemTextJsonValueConverter());
             // we cannot access this[string] with anything else than JsonObject, otherwise itw will throw
             options.Interop.TypeResolver = new TypeResolver
             {
@@ -193,4 +193,3 @@ function populateFullName() {
         Assert.True(engine.Evaluate("variables.employees.other == 'def'").AsBoolean());
     }
 }
-#endif
diff --git a/Jint.Tests/Runtime/InteropTests.cs b/Jint.Tests/Runtime/InteropTests.cs
@@ -850,7 +850,7 @@ public void CanAddArrayPrototypeForArrayLikeClrObjects()
                 .AllowClr(typeof(Person).Assembly)
                 .SetWrapObjectHandler((engine, target, type) =>
                 {
-                    var instance = new ObjectWrapper(engine, target);
+                    var instance = ObjectWrapper.Create(engine, target);
                     if (instance.IsArrayLike)
                     {
                         instance.SetPrototypeOf(engine.Realm.Intrinsics.Array.PrototypeObject);
@@ -885,7 +885,7 @@ public void CanSetIsConcatSpreadableForArrays()
             {
                 opt.SetWrapObjectHandler((eng, obj, type) =>
                 {
-                    var wrapper = new ObjectWrapper(eng, obj);
+                    var wrapper = ObjectWrapper.Create(eng, obj);
                     if (wrapper.IsArrayLike)
                     {
                         wrapper.SetPrototypeOf(eng.Realm.Intrinsics.Array.PrototypeObject);
@@ -2633,7 +2633,7 @@ public void ObjectWrapperOverridingEquality()
         [Fact]
         public void ObjectWrapperWrappingDictionaryShouldNotBeArrayLike()
         {
-            var wrapper = new ObjectWrapper(_engine, new Dictionary<string, object>());
+            var wrapper = ObjectWrapper.Create(_engine, new Dictionary<string, object>());
             Assert.False(wrapper.IsArrayLike);
         }
 

diff --git a/Jint.Tests/Runtime/ModuleTests.cs b/Jint.Tests/Runtime/ModuleTests.cs
@@ -631,7 +631,7 @@ public void EngineShouldTransmitSourceModuleForModuleLoader()
     }
     public class ModuleLoaderForEngineShouldTransmitSourceModuleForModuleLoaderTest : ModuleLoader
     {
-        public override ResolvedSpecifier Resolve(string? referencingModuleLocation, ModuleRequest moduleRequest)
+        public override ResolvedSpecifier Resolve(string referencingModuleLocation, ModuleRequest moduleRequest)
         {
             var moduleSpec = moduleRequest.Specifier;
 

diff --git a/Jint.Tests/Runtime/PropertyDescriptorTests.cs b/Jint.Tests/Runtime/PropertyDescriptorTests.cs
@@ -167,7 +167,7 @@ public void ClrAccessDescriptor()
         JsValue ExtractClrAccessDescriptor(JsValue jsArugments)
         {
             var pd = ((JsArguments) jsArugments).ParameterMap.GetOwnProperty("0");
-            return new ObjectWrapper(_engine, pd);
+            return ObjectWrapper.Create(_engine, pd);
         }
         _engine.SetValue("ExtractClrAccessDescriptor", ExtractClrAccessDescriptor);
         var pdobj = _engine.Evaluate("""

diff --git a/Jint/Options.cs b/Jint/Options.cs
@@ -125,7 +125,7 @@ internal void Apply(Engine engine)
                             new NamespaceReference(engine, TypeConverter.ToString(arguments.At(0)))),
                     PropertyFlag.AllForbidden));
 
-                engine.Realm.GlobalObject.SetProperty("clrHelper", new PropertyDescriptor(new ObjectWrapper(engine, new ClrHelper(Interop)), PropertyFlag.AllForbidden));
+                engine.Realm.GlobalObject.SetProperty("clrHelper", new PropertyDescriptor(ObjectWrapper.Create(engine, new ClrHelper(Interop)), PropertyFlag.AllForbidden));
 
 #pragma warning restore IL2026
             }
@@ -291,7 +291,7 @@ public class InteropOptions
             /// ObjectInstance using class ObjectWrapper. This function can be used to
             /// change the behavior.
             /// </summary>
-            public WrapObjectDelegate WrapObjectHandler { get; set; } = static (engine, target, type) => new ObjectWrapper(engine, target, type);
+            public WrapObjectDelegate WrapObjectHandler { get; set; } = static (engine, target, type) => ObjectWrapper.Create(engine, target, type);
 
             /// <summary>
             ///

diff --git a/Jint/Runtime/Interop/ClrHelper.cs b/Jint/Runtime/Interop/ClrHelper.cs
@@ -30,7 +30,7 @@ public JsValue ToString(JsValue value)
     public JsValue Unwrap(ObjectWrapper obj)
 #pragma warning restore CA1822
     {
-        return new ObjectWrapper(obj.Engine, obj.Target);
+        return ObjectWrapper.Create(obj.Engine, obj.Target);
     }
 
     /// <summary>
@@ -44,7 +44,7 @@ public JsValue Wrap(ObjectWrapper obj, TypeReference type)
         {
             ExceptionHelper.ThrowTypeError(type.Engine.Realm, "Argument obj must be an instance of type");
         }
-        return new ObjectWrapper(obj.Engine, obj.Target, type.ReferenceType);
+        return ObjectWrapper.Create(obj.Engine, obj.Target, type.ReferenceType);
     }
 
     /// <summary>

diff --git a/Jint/Runtime/Interop/DefaultTypeConverter.cs b/Jint/Runtime/Interop/DefaultTypeConverter.cs
@@ -395,7 +395,7 @@ internal static class ObjectExtensions
 
         public static void SetHiddenClrObjectProperty(this ObjectInstance obj, string name, object value)
         {
-            obj.SetOwnProperty(name, new PropertyDescriptor(new ObjectWrapper(obj.Engine, value), PropertyFlag.AllForbidden));
+            obj.SetOwnProperty(name, new PropertyDescriptor(ObjectWrapper.Create(obj.Engine, value), PropertyFlag.AllForbidden));
         }
     }
 }
diff --git a/Jint/Runtime/Interop/IObjectWrapper.cs b/Jint/Runtime/Interop/IObjectWrapper.cs
@@ -1,7 +1,6 @@
-namespace Jint.Runtime.Interop
+namespace Jint.Runtime.Interop;
+
+public interface IObjectWrapper
 {
-	public interface IObjectWrapper
-    {
-        object Target { get; }
-    }
+    object Target { get; }
 }
diff --git a/Jint/Runtime/Interop/ObjectWrapper.cs b/Jint/Runtime/Interop/ObjectWrapper.cs
@@ -21,6 +21,7 @@ public sealed class ObjectWrapper : ObjectInstance, IObjectWrapper, IEquatable<O
     {
         internal readonly TypeDescriptor _typeDescriptor;
 
+        [Obsolete("Use ObjectWrapper.Create instead of calling constructor directly")]
         public ObjectWrapper(
             Engine engine,
             object obj,
@@ -39,6 +40,14 @@ public ObjectWrapper(
             }
         }
 
+        /// <summary>
+        /// Creates a new object wrapper for given object instance and exposed type.
+        /// </summary>
+        public static ObjectInstance Create(Engine engine, object obj, Type? type = null)
+#pragma warning disable CS0618 // Type or member is obsolete
+            => new ObjectWrapper(engine, obj, type);
+#pragma warning restore CS0618 // Type or member is obsolete
+
         public object Target { get; }
         internal Type ClrType { get; }
 

diff --git a/Jint/Runtime/Interop/TypeReference.cs b/Jint/Runtime/Interop/TypeReference.cs
@@ -356,7 +356,7 @@ private static JsBoolean HasInstance(JsValue thisObject, JsValue[] arguments)
 
             var derivedType = other switch
             {
-                ObjectWrapper wrapper => wrapper.Target.GetType(),
+                IObjectWrapper wrapper => wrapper.Target.GetType(),
                 TypeReferencePrototype otherTypeReference => otherTypeReference.TypeReference.ReferenceType,
                 _ => null
             };