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

Enhancement: Custom equals #1969

Open
joshding opened this issue Nov 26, 2023 · 8 comments
Open

Enhancement: Custom equals #1969

joshding opened this issue Nov 26, 2023 · 8 comments

Comments

@joshding
Copy link
Contributor

joshding commented Nov 26, 2023

Enhancement request:
custom equals
allow .equals to accept a callback, for example:

const x = Map({a: 1, b:2})
const y = Map({a:1, b:5})
x.equals(y, (x, y) => x.get("a") === y.get("a"));

Consider the above for more complex structures such as Immutable Records, it would be helpful to be able to compare only certain fields within a collection. Sometimes, API responses may not be in the exact same shape as data stored in client state, and populating the immutable structures with these data that just have minor discrepancies causes .equals() to return false, when really we are interested in only certain fields.

Any thoughts on this? ^

@joshding joshding changed the title Custom equals enhancement request Enhancement: Custom equals Nov 26, 2023
@jdeniau
Copy link
Member

jdeniau commented Nov 27, 2023

Sounds a nice feature to have, especially if Map does already implement this!

@bdurrer
Copy link
Contributor

bdurrer commented Nov 27, 2023

collection.equals is the same (at least for the user) as Immutable.is(), would probably have to change both.

Implementing a real custom equals might become annoying quickly when using nested collections, mixed types or ordered collections.

The "normal" equals does not need to care about depth and path of the comparison, but the custom callback would need to care for that:
x.get("a") === y.get("a") might be fine on first level, but then it goes into the next depth level and a might be meaningless.

Fun fact:
the example could be replaced by x.get("a") === y.get("a");. But === isn't a good operator if "a" is an immutable object itself, it would have to rely on the immutable is or equals itself: x.get("a").equals(y.get("a"));

@joshding
Copy link
Contributor Author

joshding commented Nov 28, 2023

Ah I see, that's good to know! If equals only cares about the first level, then perhaps an array of keys to omit on the first level would be acceptable?
something like

const x = Map({a: 1, b:2})
const y = Map({a:1, b:5})
x.equals(y, ["b"]); // true, only "a" is compared

Just as a side note, I am trying to run npm run test on this repo locally, but have this error message pop up, seems to be from dtslint dependency. Anywhere I can go to resolve these issues or help get up and running? my node version is 20.5.0 and npm version is 10.2.4. Thanks in advance
Screenshot 2023-11-27 at 6 14 43 PM

update: found this knex/knex#5631
but i tried with node 14, node 18.15.0, and node 20, and they all have same error

@bdurrer
Copy link
Contributor

bdurrer commented Nov 28, 2023

Ah I see, that's good to know! If equals only cares about the first level, then perhaps an array of keys to omit on the first level would be acceptable? something like

Sorry, that's a misunderstanding: What I wanted to say is that the internal deepEquals does not need to care about depth, it is simply called recursively and applies equals on everything. A single miss results in a false.

But for an end-user to implement a custom equals, depth is likely important and probably annoying to deal with.
To deal with such a "similar-ish" comparison, I believe that a simple forEach or x.get('b') === y.get('b') is likely easier.

But feel free to provide a Pull-Request 😁

@jdeniau
Copy link
Member

jdeniau commented Nov 28, 2023

Sounds a nice feature to have, especially if Map does already implement this!

Sorry I misread your initial post. I though that Map did implement this already!

Just to understand the need here : is there a, real difference between your proposal

a.equals(b, (a, B) => a.get('foo') === b.get('foo'))

And an external function in your code?

const compare Foes = (a, B) => a.get('foo') === b.get('foo'))

compareFoos(a, b) 

What I would like to put here is : is this really an immutable feature that might be useful for everyone or non?

@joshding
Copy link
Contributor Author

joshding commented Nov 29, 2023

Thanks for your input!!
Was able to play around with the deepEquals method a bit. Probably my initial idea of a callback would be quite complicated and less useful. But perhaps an optional array of keys to omit would be a better idea?

For example:

    const list1 = List([Map({a:1,b:2,c:3}), 4,5]);
    const list2 = List([Map({a:10,b:11,c:3}), 4,5])
    expect(list1.equals(list2, ["a", "b"])).toBe(true);

implementation-wise, although deepEquals is not aware of depth, it seems to be aware of the key, where we can skip the iteration of comparison for any keys matching the "omit" array option, regardless of depth.

The use case could possibly be what I had initially mentioned, sometimes we hydrate models with data from the API on update , where fields that we did not initially have (since we didn't fetch them on page load) are populated with data. However, it could be convenient to have an option to compare equality only a subset of the fields within the models with each other. It doesn't seem like a trivial task to modify deepEquals, but I guess that's why I wanted to discuss a bit before investing too much time into it :D Thanks again for your insights!

@jdeniau
Copy link
Member

jdeniau commented Nov 29, 2023

For the record, we have a similar case, and knowing the keys, we do use the Record structure, which is slightly easier in that case : you can use get or other immutable methods, but you can also use it as a plain object :

myRecord.someKey

@bdurrer
Copy link
Contributor

bdurrer commented Nov 30, 2023

Just a side

    const list1 = List([Map({a:1,b:2,c:3}), 4,5]);
    const list2 = List([Map({a:10,b:11,c:3}), 4,5])
    expect(list1.equals(list2, ["a", "b"])).toBe(true);

neither list does have a or b, only an element at position 0, test should fail.

How would we even decide when a list is equal? If it has more than one element, relevance of order and position are very much dependent on your personal use case. Is it still equals when one has more elements than the others? How to figure out which ones fit together?

As Julien mentioned, shallowly comparing records and maps is easy with some/someKey:

const isNotEquals = ["a", "b"].some((key) => mapA.get(key) !== mapB.get(key))

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

No branches or pull requests

3 participants