-
-
Notifications
You must be signed in to change notification settings - Fork 231
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
Prototype deep equality #2258
base: main
Are you sure you want to change the base?
Prototype deep equality #2258
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could do it regionally:
const Thing = Data.withDeepEquality(() => Data.tagged<Thing>())
Adding an options param everywhere feels a bit clunky.
} | ||
|
||
function deepComp(this: any, that: any) { | ||
if (typeof that === typeof this && that !== null && this[deepSymbol] === that[deepSymbol]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (typeof that === typeof this && that !== null && this[deepSymbol] === that[deepSymbol]) { | |
if (typeof that === typeof this && that !== null && this[deepSymbol] && that[deepSymbol]) { |
Only if both have deep equality enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, equality should be symmetric no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wasn't sure if setting deep to false on both should pass the condition.
Tried out the regional idea: https://github.com/Effect-TS/effect/pull/2259/files |
I feel like regional would be far too verbose and it's not a solution for classes as you can't creste a class inside the region |
@tim-smart I think we need to make a choice here, either we add the param to the api or we decide it's not important. WDYT? cc @IMax153 @gcanti @fubhy |
against adding params |
Both seem a bit awkward tbh. Also, apologies in advance as I'm probably missing something here, but why would you ever want to use the Also, the name isn't great. The consumers shouldn't care that it uses a proxy. The name should reflect what it does, not how. |
I think deep equality is something worth pursuing, as a beginner will likely expect Equal.equals to also check the import { Data } from "effect"
class User extends Data.Class<{ name: string; metadata: { foo: string } }> {} But I wonder if we tackle it in the constructors rather than in the actual Equal implementation. I.e. ensure any nested objects implement Equal & Hash. |
import { Data } from "effect"
class User extends Data.Class<{ name: string; metadata: { foo: string } }>({deep: true}) {} I don't see any single reason not to add parameters. Constructors don't exist for plain objects for example The decision here is:
If anyone has concrete concerns about adding params that are not "I am against" please explain with examples and reasons |
Having an object / array already constructed and wanting to have a view (proxy) which compares by value instead of by reference |
If the issue is unexpected behavior, adding an opt-in parameter |
Yes, I get that, but my question was why would you ever want that then to NOT perform deep comparison. As Giulio also said, it should at least be deep by default then. I'm just additionally questioning if you'd ever want it to NOT perform deep comparison, hence wondering if we can just drop that option entirely. |
We can make it default if we have an opt-out. Considering everything by value is a mistake, one could have mutable objects and objects with circular dependencies. We once tried deep by default and had to back-off due to issues |
Note: the issue is not unexpected behaviour, it's absolutely expected not to have deep comparison if you use mutable data such as nested objects. But that doesn't mean the user wouldn't want deep comparison, this is about making it easier to do it instead of having to declare nested classes |
we could even think of something like: import { Data } from "effect"
class User extends Data.Class<{
name: string;
metadata: { foo: string };
a: { b: MutableStuff } }
>({ byRef: ["a.b"] }) {} |
Could this be an option too? import { Data } from "effect"
class User extends Data.Class<{
name: string;
metadata: { foo: string };
a: { b: MutableStuff } }
({
a: {
b: MyCustomCmpFn // Explicitly override the default behavior here.
}
}) {} |
it's not only a comparator function but also a hashing implementation, not sure if it would be possible, also I would have no idea how to write the type of the parameter |
Alternative idea: we could use decorators instead of parameters for classes I think we need to approach this in steps:
|
I think decorators could be nice, you could use them for both classes and the other apis from what I recall. I remember trying to use them when trying out apis for making Hash lazy, but ran into some issues with tsconfig. |
What other APIs do you mean? |
If we decide that this goes in the right direction we'll have to extend the rest of constructors to allow deep comparison.
cc @tim-smart for further testing & potential api extensions
linked discord discussion: https://discord.com/channels/795981131316985866/795983589644304396/1214495832246853682
closes #2251