Page MenuHomePhabricator

[lib] introduce function to find deep difference between two objects
ClosedPublic

Authored by kamil on May 9 2023, 3:17 AM.
Tags
None
Referenced Files
F3409296: D7753.id27155.diff
Wed, Dec 4, 3:37 PM
F3409261: D7753.id26274.diff
Wed, Dec 4, 3:25 PM
F3409258: D7753.id.diff
Wed, Dec 4, 3:25 PM
F3409257: D7753.id26274.diff
Wed, Dec 4, 3:25 PM
F3409256: D7753.id27258.diff
Wed, Dec 4, 3:24 PM
F3409254: D7753.id27256.diff
Wed, Dec 4, 3:24 PM
F3409253: D7753.id27155.diff
Wed, Dec 4, 3:24 PM
F3409117: D7753.diff
Wed, Dec 4, 3:01 PM
Subscribers

Details

Summary

Implement function returning objects with properties that are included in first object but not included in second one.

Flow doesn't like generic Map<K, T> while iterating over properties, so I used Object.

Depends on D7752

Test Plan

Run tests

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.May 9 2023, 3:32 AM
lib/utils/objects.js
57 ↗(On Diff #26274)

You might be able to use $Rest

Flow doesn't like generic Map<K, T> while iterating over properties, so I used Object.

Can you please provide more details on this issue? Object is basically any. Consider that ENG-3806 wouldn't have happened if Flow was able to realize that undefined is not a valid Object.

Looking for:

  1. Longer description of Flow's issues here
  2. Things you've tried
  3. Links to GitHub issues on the Flow repo explaining Flow's problem here

Note that Map<K, T> defines the Map class in JS, and doesn't work for objects. If you absolutely must use an object, you should still be able to type it better, eg. { +[key: string]: mixed } or something

tomek requested changes to this revision.May 16 2023, 3:36 AM
tomek added inline comments.
lib/utils/objects.js
57 ↗(On Diff #26274)
This revision now requires changes to proceed.May 16 2023, 3:36 AM
kamil requested review of this revision.May 25 2023, 2:20 AM

Looking for:

  1. Longer description of Flow's issues here

Map type is defined as follows in the codebase: type Map<K, T> = { +[key: K]: T };

Issues:

  1. I can not iterate over obj1 or obj2 using for..in, reason: string [1] is incompatible with K [2]. [incompatible-type].
  2. I can not use Object.keys() or Object.entries() properly as they return string, not generic K as key type. See the definition in core.js
  3. I can not assign differences to result object, with any method, reason: Cannot assign computed property using K [1]. [invalid-computed-prop] or Cannot use in because on the left-hand side, K [1] must be a string or number. [invalid-in-lhs].
  4. I can not call this function recursive without casting via any: Cannot call deepDiff with obj2[key] bound to obj2 because T [1] is incompatible with Map [2]. [incompatible-call]
  1. Things you've tried

Issues 1, 2: using lodash which (according to flow-typed) allows generic key, e.g. _keys function, but this entire function should be based on only lodash utilities.
Issues 3, 4: I don't have a solution.

Typing objects as { +[key: string]: mixed }:

  1. mixed is incompatible with { +[key: string]: mixed } - require casting
  2. This will requires also changing types of assertObjectsAreEqual<K, T>() as arguments have different types now.

I also tried different configurations with different generic types - same results.
This also applies to @tomek and @michal suggestions.

  1. Links to GitHub issues on the Flow repo explaining Flow's problem here

I linked to the core.js definition, and I think the rest of problems is obvious.

I don't think using Object (or even any) is a serious issue in this context. I feel like we trying to type some runtime things and it will never be accurate and always will require some casting, e.g. via any. Other function in this file also are based on Object and seems fine (findMaximumDepth or hash). Even if someone will call this function with some weird values, e.g. deepDiff(undefined, 3.14) the result will be {} which for me is suitable with a definition: returns an object with properties from obj1 not included in obj2. (there is a bug in this code that makes this function not return proper result for deepDiff([undefined, 4], 3.14) - but that's my oversight)

Requesting review to see your point of view.

ashoat requested changes to this revision.May 25 2023, 4:11 AM

Map type is defined as follows in the codebase: type Map<K, T> = { +[key: K]: T };

Ah, I got confused here... I think we should rename this type (see inline comment).

Issues:

  1. I can not iterate over obj1 or obj2 using for..in, reason: string [1] is incompatible with K [2]. [incompatible-type].
  2. I can not use Object.keys() or Object.entries() properly as they return string, not generic K as key type. See the definition in core.js
  3. I can not assign differences to result object, with any method, reason: Cannot assign computed property using K [1]. [invalid-computed-prop] or Cannot use in because on the left-hand side, K [1] must be a string or number. [invalid-in-lhs].
  4. I can not call this function recursive without casting via any: Cannot call deepDiff with obj2[key] bound to obj2 because T [1] is incompatible with Map [2]. [incompatible-call]

Thanks for explaining!! I think option 2 is the easiest to solve. It's a common issue in our codebase, and we have a solution in the very objects.js file you're testing here. (We basically solve it with an any-cast... sometimes those are necessary, but like unsafe blocks in Rust, we try to be very "targeted" with how we use those, and isolate it to reusable utilities instead of leaving it in business logic.)

(By the way, that Flow issue has a GitHub issue, where this commit is mentioned, which solves the problem starting with Flow 0.196, which will be available in React Native 0.72 – currently in release candidate phase.)

I don't think using Object (or even any) is a serious issue in this context. I feel like we trying to type some runtime things and it will never be accurate and always will require some casting, e.g. via any. Other function in this file also are based on Object and seems fine (findMaximumDepth or hash). Even if someone will call this function with some weird values, e.g. deepDiff(undefined, 3.14) the result will be {} which for me is suitable with a definition: returns an object with properties from obj1 not included in obj2. (there is a bug in this code that makes this function not return proper result for deepDiff([undefined, 4], 3.14) - but that's my oversight)

Requesting review to see your point of view.

I don't disagree. But without seeing everything you tried, it's hard for a reviewer to give advice. Sometimes there is an "easy" solution available that lets us type things better, and by pushing on this it might give you more context on how to resolve issues like this in the future, where it might be more of an issue.

lib/utils/objects.js
10 ↗(On Diff #26274)

Can we avoid shadowing the built-in Map type? It can be confusing for the reader, who assumes you're using the built-in type. We can name this something like ObjectMap maybe

This revision now requires changes to proceed.May 25 2023, 4:11 AM

Thanks for your response @ashoat. Adding the alternative solution I tried, using generics.

lib/utils/objects.js
49–51 ↗(On Diff #27155)

this fixes the problem that Object.keys() returns Array<string>, not K[]. (see core definition)

53–62 ↗(On Diff #27155)

This allows assigning something to an object using computed K.

We can not directly use obj[K] = sth; because [invalid-computed-prop] error

87–88 ↗(On Diff #27155)

This is needed to make obj1[key] (which is a type of T) compatible with ObjectMap<K, T>, but because both obj1[key] and obj2[key] are plain objects (see the condition in line 82) this casting should be safe

92 ↗(On Diff #27155)

This casting can be avoided when we update generic type to
type ObjectMap<K, T> = { +[key: K]: T | ObjectMap<K, T> };,
and fix problems in other functions in this file, eg. in
values<K, T>(map: ObjectMap<K, T>): T[] function - not sure which definition is better.

10 ↗(On Diff #26274)

Done!

Thanks for annotating everything. I tried it locally and couldn't figure out anything better, unfortunately

lib/utils/objects.js
92 ↗(On Diff #27155)

I like that suggestion, but instead of updating ObjectMap, can you introduce a new NestedObjectMap that you could use for just this function (and any functions in dependant diffs that will need to call it)?

introduce NestedObjectMap type

lib/utils/objects.js
92 ↗(On Diff #27155)

sure

Removing @tomek to unblock this because he does not work this week and his review is no longer applicable as this function was typed differently than in the previous version.

This revision is now accepted and ready to land.May 30 2023, 3:34 AM