Prototype deep equality

[mikearnaldi]

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

[changeset-bot]

⚠️ No Changeset found

Latest commit: 5419787

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[tim-smart]

I wonder if we could do it regionally:

const Thing = Data.withDeepEquality(() => Data.tagged<Thing>())

Adding an options param everywhere feels a bit clunky.

[tim-smart]

Suggested change

	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?

[mikearnaldi]

yes, equality should be symmetric no?

[tim-smart]

Just wasn't sure if setting deep to false on both should pass the condition.

[tim-smart]

Tried out the regional idea: https://github.com/Effect-TS/effect/pull/2259/files

[mikearnaldi]

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

[mikearnaldi]

@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

[gcanti]

against adding params

[fubhy]

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 Data.proxy() and then set deep to false? Isn't the whole point of it to support deep equality?

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.

[tim-smart]

I think deep equality is something worth pursuing, as a beginner will likely expect Equal.equals to also check the metadata object in this example:

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.

[mikearnaldi]

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 Data.tagged and in classes it would require typing parameters which would look horrendous.

The decision here is:

1. we care about deep equal => we add params
2. we don't care about deep equals => everything remains the same

If anyone has concrete concerns about adding params that are not "I am against" please explain with examples and reasons

[mikearnaldi]

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 Data.proxy() and then set deep to false? Isn't the whole point of it to support deep equality?

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.

Having an object / array already constructed and wanting to have a view (proxy) which compares by value instead of by reference

[gcanti]

If the issue is unexpected behavior, adding an opt-in parameter { deep: true } doesn't solve the problem, it should be the default behavior.

[fubhy]

Having an object / array already constructed and wanting to have a view (proxy) which compares by value instead of by reference

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.

[mikearnaldi]

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

[mikearnaldi]

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

[mikearnaldi]

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"] }) {}

[fubhy]

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.
  }
}) {}

[mikearnaldi]

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

[mikearnaldi]

Alternative idea: we could use decorators instead of parameters for classes

I think we need to approach this in steps:

1. provide a good way of creating deeply compared data classes, ideally via a decorator for backward compatibility

2. long term re-evaluate deep equality by default with the caveat of not deeply comparing custom typed but only plain structures, this if it ever lands will be material for 4.0 after a good period of testing

[tim-smart]

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.

[mikearnaldi]

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?