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
Details
Details
Run tests
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- deep-diff
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
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:
- Longer description of Flow's issues here
- Things you've tried
- Links to GitHub issues on the Flow repo explaining Flow's problem here
Comment Actions
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
| lib/utils/objects.js | ||
|---|---|---|
| 57 ↗ | (On Diff #26274) | Wondering if we can use $Diff https://flow.org/en/docs/types/utilities/#toc-diff |
Comment Actions
Map type is defined as follows in the codebase: type Map<K, T> = { +[key: K]: T };
Issues:
- I can not iterate over obj1 or obj2 using for..in, reason: string [1] is incompatible with K [2]. [incompatible-type].
- 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
- 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].
- 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]
- 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 }:
- mixed is incompatible with { +[key: string]: mixed } - require casting
- 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.
- 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.
Comment Actions
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:
- I can not iterate over obj1 or obj2 using for..in, reason: string [1] is incompatible with K [2]. [incompatible-type].
- 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
- 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].
- 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 |
Comment Actions
Thanks for your response @ashoat. Adding the alternative solution I tried, using generics.
| lib/utils/objects.js | ||
|---|---|---|
| 10 ↗ | (On Diff #26274) | Done! |
| 49–51 | this fixes the problem that Object.keys() returns Array<string>, not K[]. (see core definition) | |
| 53–62 | 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 | 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 | This casting can be avoided when we update generic type to | |
Comment Actions
Thanks for annotating everything. I tried it locally and couldn't figure out anything better, unfortunately
| lib/utils/objects.js | ||
|---|---|---|
| 92 | 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)? | |
Comment Actions
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.