Mapping between .Net objects and JSReference should not allow to match objects of different types.

[dmitriyse]

The problem can be reproduced as follows:

1. Creating a class (let name it MyObj) with overridden Object.Equals that matches different instances
2. Creating instance A of MyObj
3. Creating instance B of MyObj which MyObj.Equals matches to instance A
4. Passing instance A to JS (for example as a return from a Method)
5. Passing instance B to JS
6. Problem: JS will receive instance A two times, and it's no any way to pass instance B to JS

This fix implements object comparison that doesn't match objects of different types, even if the Equals(object) method matches another type.
Plus, it allows the replacement of the comparer if, for some reason, the default comparer implementation does not work well with an Equal(object) implementation of some type.

[dmitriyse]

@microsoft-github-policy-service agree

[jasongin]

@dmitriyse Thanks for the PR! I understand the issue this is trying to fix, and it makes sense for reference types.

However I'm not sure about value types (structs). There is a test failure when checking JS equality of Version, which is a struct:

Failed Microsoft.JavaScript.NodeApi.Test.HostedClrTests.Test(id: "napi-dotnet/dynamic_invoke") [767 ms]
  Error Message:
   Assert.Fail(): node:assert:1[25](https://github.com/microsoft/node-api-dotnet/actions/runs/6336476172/job/17479606049?pr=156#step:12:26)
  throw new AssertionError(obj);
  ^
AssertionError [ERR_ASSERTION]: Values have same structure but are not reference-equal

You can run just that test case with the command:

dotnet test -f net8.0 --filter "DisplayName~dynamic_invoke"

.NET structs are boxed when wrapped by JS, or when stored or compared in that dictionary. So every time a value is boxed it gets a new object instance, meaning reference equality doesn't work.

Arguably since Version is a .NET value-type, the JS equality check should maybe be checking only for value equality and not reference equality. But there are no "value types" in JS, so I think the expectation of a JS developer would be that an object that is passed through .NET and back to JS should still be the same JS object instance. What do you think?

More broadly, I'm curious: what is your interest in this project, and how do you plan to use it?

[dmitriyse]

Hi @jasongin,
Thank you for the feedback and tips for debugging.

I am working on the bridge for the testing framework that propagates FlaUI API to Node.JS

I can implement a custom comparison that falls back to the structural Equality for:

• Value Types
• C# Strings

I think it's hard to find a solution for object mapping that doesn't have any side effects. Once we discover some unwanted side effects, we will try to improve the solution.

I will prepare the update of this PR in the next couple of days.

[jasongin]

I am working on the bridge for the testing framework that propagates FlaUI API to Node.JS

Cool! Feel free to open issues about any challenges you encounter in doing that. And I would love to see a demo when you have something ready to share.

I can implement a custom comparison that falls back to the structural Equality for: Value Types, C# Strings

That sounds like it should work well.

[dmitriyse]

@jasongin, can you please review the updated implementation of this PR?
I have changed the approach to fix the problem. Mapping logic will not return unexpected types to JS, as different types will never match some existing mapping.

[jasongin]

This doesn't seem right, and is different from what was discussed. It will return the wrong result for two (equal) instances of the same class, as in the original problem scenario you described.

If x is a reference type then we want to use reference equality, as in the original iteration of this PR. If x is a (boxed) value-type then we want to use value equality. (Value equality should fail if the values are different types.)

I think this should work, but it might need some testing:

return x is ValueType ? x.Equals(y) : Object.ReferenceEquals(x, y);

[jasongin]

Superseded by PR #247

The implementation in the newer PR is basically the same as the first iteration of this one, though I added tests.

I apologize for my confusion in the discussion above. I had been thinking Version is a struct, but it is actually a class. So I had misinterpreted the failed test case related to marshalling Version. The object map is not used at all for structs, so there was no need to handle value-types in its equality comparer.