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

Remove exposed CLR type equality requirement from ObjectWrapper.Equals #1652

Merged
merged 1 commit into from
Oct 21, 2023

Conversation

lahma
Copy link
Collaborator

@lahma lahma commented Oct 21, 2023

I can't wrap my head around why we would need to have Equals to check object wrapper's exposed type as current ReferenceEquals is already a strong guarantee. There might be cases to only expose member of exposed type but we shouldn't state that pointer to same object isn't the same object based on what was exposed.

This should return the behavior expected with @CreepGin 's Unity integration.

/cc @CreepGin @viruscamp

@viruscamp
Copy link
Contributor

Actually, It's a design choice.
I selected type Equals, because even same CLR object with different type may have different behavior.
Additional I think it should use == to check without type, and === to check with type.

@lahma
Copy link
Collaborator Author

lahma commented Oct 21, 2023

In .NET ReferenceEquals always wins and this is interop, in JavaScript there's no such comparison as the inheritance model is just so different. So I guess this change of behavior back to original is still a design choice too? For me it makes more sense.

@lahma
Copy link
Collaborator Author

lahma commented Oct 21, 2023

I'll merge this to create a new release, we can iterate based on more feedback if needed.

@lahma lahma merged commit f4667ed into sebastienros:main Oct 21, 2023
3 checks passed
@lahma lahma deleted the interop-objectwrapper-equality branch October 21, 2023 17:00
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