Make JsSet public #1987

kenlyon · 2024-10-24T17:52:36Z

Allow use of the Jint.Native.JsSet class outside of expressions, allowing consumers to cast to this type and call its members.

For example, you might want to convert it to a HashSet<T> to use it elsewhere without being tied to using Jint types.

The OrderedSet class also had to be made public due to its use in the JsSet constructor.

lahma · 2024-10-24T18:03:47Z

Jint/Native/JsSet.cs


-    internal bool SetDelete(JsValue key) => _set.Remove(key);
+    public bool SetDelete(JsValue key) => _set.Remove(key);


Remove (which should be Delete) already handles this so can be removed altogether.

I kept this name for now given that "Delete" is inherited. Alternatively I could use new to hide the inherited version although I think that's generally discouraged.

lahma · 2024-10-24T18:04:53Z

Jint/Native/JsSet.cs

@@ -73,7 +73,7 @@ internal void ForEach(ICallable callable, JsValue thisArg)
        _engine._jsValueArrayPool.ReturnArray(args);
    }

-    internal ObjectInstance Entries() => _engine.Realm.Intrinsics.SetIteratorPrototype.ConstructEntryIterator(this);
+    public ObjectInstance Entries() => _engine.Realm.Intrinsics.SetIteratorPrototype.ConstructEntryIterator(this);


Are Entries and Values really needed, they don't expose meaningful API outside of Jint's logic at the moment?

No, right enough. They're not really needed elsewhere.

Changed back to internal.

lahma · 2024-10-24T18:05:36Z

Jint/Runtime/OrderedSet.cs

@@ -1,6 +1,6 @@
 namespace Jint.Runtime;

-internal sealed class OrderedSet<T>
+public sealed class OrderedSet<T>


I think this can remain as internal, unfortunate need for JS usage where they come up with oddities

So SetConstructor would handle all constructor requirements and JsSet constructor should be internal, like I indicated in earlier discussion, SetConstructor.Construct() would have the needed overloads

Unfortunately, I wasn't able to compile without this change. I think the alternative would be to downgrade the JsSet constructor that uses it to also be internal. This could potentially be a breaking change as I don't know where all it is used.

I was able to revert this by changing the affected JsSet constructor to internal. Hopefully that doesn't break something elsewhere. I only see it used by SetPrototype.

lahma · 2024-10-24T18:06:54Z

Jint/Native/JsSet.cs


-    internal void Remove(JsValue value) => _set.Remove(value);
+    public void Remove(JsValue value) => _set.Remove(value);


I think this should be just plain Delete, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set

You mean rename the method itself to Delete(JsValue value)? Given that it was previously internal, am I safe to assume that all existing consumers that could break as a result are in the same repo? Hopefully refactoring it globally is adequate.

OK, so I see it's only used in SetPrototype, however I wonder if the name "SetDelete" was used to avoid conflicting with ObjectInstance.Delete() which would attempt to delete a property by that name from the object (presumably like javascript's delete foo.bar) whereas we're wanting mySet.delete("abc") to take a value out of the set.

lahma · 2024-10-24T18:08:03Z

Jint/Native/JsSet.cs


-    internal void ForEach(ICallable callable, JsValue thisArg)
+    public void ForEach(ICallable callable, JsValue thisArg)


Maybe better keep this internal, very JS spec specific and not a natural c# API

Done. This also eliminates a warning I was getting because ICallable is internal.

…ete => Remove

lahma

I did small adjustments and a public API test to show usage (and ensure API will be kept), did a 180 and went with the Remove method name as SetDelete felt too clunky...

lahma · 2024-10-26T05:58:00Z

Thanks for iterating a publishing the API!

lahma · 2024-10-26T16:19:21Z

In #1988 I did another 180 and came to conclusion that in this case the new hiding actually makes sense as you are working with Set and Map semantics with direct type, you probably don't want to do "generic object operations". So JsSet.Delete now maps to Set.prototype.delete.

sebastienros · 2024-10-26T19:47:39Z

Jint/Runtime/OrderedSet.cs

 namespace Jint.Runtime;

-internal sealed class OrderedSet<T>
+internal sealed class OrderedSet<T> : IEnumerable<T>
 {
    internal List<T> _list;


OrderedDictionary<T, T> has been added recently, or is it soon? https://learn.microsoft.com/en-us/dotnet/api/system.collections.specialized.ordereddictionary?view=net-8.0

The generic version ( https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.ordereddictionary-2 ) has been added NET 9, maybe not yet worth to add another TFM just for that..

That's what was confusing me then. I knew the generic one was added to 9, but on the docs it said 2,3....9. I was pointing to the wrong one. It all makes sense and I agree not worth it if just for 9.

kenlyon · 2024-10-28T17:44:18Z

Thanks for finishing this. I've upgraded to Jint 4.1.0 and these changes are really helpful.

Make JsSet public

9fc2663

kenlyon mentioned this pull request Oct 24, 2024

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

Closed

lahma requested changes Oct 24, 2024

View reviewed changes

kenlyon and others added 3 commits October 24, 2024 13:33

Some code review changes

d2301a7

Merge branch 'main' into make-jsset-public

8adcb11

Use intrinsics to construct, add public API test, IEnumerable, SetDel…

df78932

…ete => Remove

lahma approved these changes Oct 26, 2024

View reviewed changes

lahma merged commit 7d1de2d into sebastienros:main Oct 26, 2024
3 checks passed

sebastienros reviewed Oct 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make JsSet public #1987

Make JsSet public #1987

kenlyon commented Oct 24, 2024

lahma Oct 24, 2024

kenlyon Oct 24, 2024

lahma Oct 24, 2024

kenlyon Oct 24, 2024

kenlyon Oct 24, 2024

lahma Oct 24, 2024

lahma Oct 24, 2024

kenlyon Oct 24, 2024

kenlyon Oct 24, 2024

lahma Oct 24, 2024

kenlyon Oct 24, 2024

kenlyon Oct 24, 2024

lahma Oct 24, 2024

kenlyon Oct 24, 2024

lahma left a comment

lahma commented Oct 26, 2024

lahma commented Oct 26, 2024 •

edited

Loading

sebastienros Oct 26, 2024

lahma Oct 27, 2024

sebastienros Oct 28, 2024

kenlyon commented Oct 28, 2024


		internal bool SetDelete(JsValue key) => _set.Remove(key);
		public bool SetDelete(JsValue key) => _set.Remove(key);


		internal void Remove(JsValue value) => _set.Remove(value);
		public void Remove(JsValue value) => _set.Remove(value);


		internal void ForEach(ICallable callable, JsValue thisArg)
		public void ForEach(ICallable callable, JsValue thisArg)

Make JsSet public #1987

Make JsSet public #1987

Conversation

kenlyon commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lahma left a comment

Choose a reason for hiding this comment

lahma commented Oct 26, 2024

lahma commented Oct 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenlyon commented Oct 28, 2024

lahma commented Oct 26, 2024 •

edited

Loading